Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

install diff-filter-buildkite-plugin to only build when relevant files changed #400

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

install diff-filter-buildkite-plugin to only build when relevant files changed #400

wants to merge 20 commits into from

Conversation

fatteneder
Copy link
Member

@fatteneder fatteneder commented Nov 2, 2024 •
edited
Loading

Fixes #243

This adds a filter that skips (most of the) build and test steps in the CI pipeline if a PR only changed files that are irrelevant for Julia's functionality.
In particular, changes to any of these files won't trigger a build (glob patterns as seen from the root of the Julia repo):

  • "*.md"
  • ".*"
  • "julia.spdx.json"
  • "CITATION.*"
  • "typos.toml"

Note that we always build for x86_64-linux-gnu in order to generate the docs.

The filter can be disabled on a PR basis by adding a label with the name ci-force-build to it.

The addition of more fine grained filters is left for future PRs.

@fatteneder fatteneder force-pushed the fa/diff-filter-build branch from e0bd514 to 36db0ee Compare November 2, 2024 17:53
@IanButterworth
Copy link
Member

I believe NEWS.md and HISTORY.md are in the docs, so I think it needs at least the docs to be built for those?

@fatteneder fatteneder force-pushed the fa/diff-filter-build branch 6 times, most recently from 9b18b56 to df7d25c Compare November 2, 2024 18:55
@fatteneder fatteneder force-pushed the fa/diff-filter-build branch 4 times, most recently from 617363c to 014c2cc Compare November 5, 2024 19:46
@fatteneder fatteneder closed this Nov 5, 2024
@fatteneder fatteneder reopened this Nov 5, 2024
@fatteneder fatteneder force-pushed the fa/diff-filter-build branch 4 times, most recently from e12afd6 to 9ccf4d5 Compare November 5, 2024 20:19
@fatteneder
Copy link
Member Author

I think I managed to implement a mechanism to override the filter using Github labels and the github-pr-labels-buildkite-plugin.
If someone could add a label with the name ci-force-build to this PR, then it should build everything again.

I think the basic functionality is now there.
Currently it contains lots of boilerplate code in various places, so I would like to refactor it.

  • Is there a simple way to share the filter definition between all relevant *.yml files?
  • Is it possible to run the diff-filter plugin once before all steps and just export the trigger to the remaining steps?

giordano
giordano reviewed Nov 6, 2024
View reviewed changes
Copy link
Member

@giordano giordano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if this is what the ignore-list is for (it's quite short, so can't tell it's what I'm thinking of), but for changes which involve only any of

shouldn't run any CI here at all. After this, for docs-related changes I think we need probably to build for all platforms but not run any test suites apart from the doctests? Not sure if this is what #400 (comment) was suggesting.

pipelines/main/launch_unsigned_jobs.yml
@@ -12,6 +12,25 @@
# and only need to touch the webui configuration when we need to alter
# something about the privileged steps.

common:
- diff-filter-build_plugin: &diff-filter-build
https://github.com/fatteneder/diff-filter-buildkite-plugin#main:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this should be moved to this organisation before merge.

pipelines/main/launch_unsigned_jobs.yml Outdated Show resolved Hide resolved
@DilumAluthge
Copy link
Member

  • manually ignoring the filter for pre-releases and tags,
  • ignore the filter when merging into master.

This can likely be simplified to: apply the filter iff the repo is JuliaLang/julia and it is a PR.

@fatteneder
Copy link
Member Author

After this, for docs-related changes I think we need probably to build for all platforms but not run any test suites apart from the doctests? Not sure if this is what #400 (comment) was suggesting.

Following #400 (comment) we now always build x86_64-linux, and then use that to run deploy_docs.yml, build_pdf_docs.yml and doctest.yml.

@fatteneder
Copy link
Member Author

This can likely be simplified to: apply the filter iff the repo is JuliaLang/julia and it is a PR.

Ok, I did not yet test how the plugin behaves once a PR is merged.

About juliaup and the +pr feature: Not exactly sure how that works, does that just grab any artifact that is cached in buildkite? If so, then with this PR here one can no longer grab a build from a "filtered" CI run, unless one adds the ci-force-build label.

@fatteneder fatteneder force-pushed the fa/diff-filter-build branch from fc89bd0 to 8dbfb8d Compare November 6, 2024 14:18
@fatteneder
Copy link
Member Author

I have not permissions on this repo/in this org. Can someone please add the ci-force-build label here for testing?

@fatteneder fatteneder added the ci-force-build Force to run all steps in the CI label Nov 6, 2024
@fatteneder fatteneder closed this Nov 6, 2024
@fatteneder fatteneder reopened this Nov 6, 2024
@DilumAluthge
Copy link
Member

Closing and reopening a PR doesn't retrigger build kite unfortunately. I asked Bill kite Support about that a while back, and they told me that was is the intended behavior.

you'll need to go to the build kite build and click the rebuild button.

@DilumAluthge
Copy link
Member

@fatteneder Can you resolve the merge conflicts and rebase?

@DilumAluthge
Copy link
Member

@gbaraldi Would you be able to review this?

@DilumAluthge DilumAluthge mentioned this pull request Dec 2, 2024
Merged
@DilumAluthge
Copy link
Member

@fatteneder Can you also squash everything into one commit?

@DilumAluthge DilumAluthge mentioned this pull request Dec 2, 2024
Open
@fatteneder fatteneder mentioned this pull request Dec 2, 2024
Closed
@fatteneder fatteneder force-pushed the fa/diff-filter-build branch from 8df10ec to f300c57 Compare December 2, 2024 12:26
@fatteneder fatteneder force-pushed the fa/diff-filter-build branch 2 times, most recently from bdd6d02 to 5973d18 Compare December 2, 2024 13:49
@fatteneder fatteneder force-pushed the fa/diff-filter-build branch from 3f540fe to 7627e90 Compare December 2, 2024 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DilumAluthge DilumAluthge DilumAluthge left review comments

@IanButterworth IanButterworth Awaiting requested review from IanButterworth

@giordano giordano Awaiting requested review from giordano

@StefanKarpinski StefanKarpinski Awaiting requested review from StefanKarpinski

@gbaraldi gbaraldi Awaiting requested review from gbaraldi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Run only a subset of tests on PRs that only modify the manual
4 participants
@fatteneder @IanButterworth @DilumAluthge @giordano