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.

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

check that patches are not broken #12457

Merged
merged 2 commits into from Mar 17, 2024

Conversation

mockersf
Copy link
Member

Objective

Solution

  • Apply all patches then build Bevy

@mockersf mockersf added the A-Build-System Related to build systems or continuous integration label Mar 13, 2024
@rparrett
Copy link
Contributor

rparrett commented Mar 13, 2024

I was slightly worried about contributor experience here. A failure currently looks like:
image
Where

  • CI doesn't tell you which patch failed, so that needs to be cross-referenced
  • CI stops after the first failure

But with this running on merge_group, I guess it's mostly maintainers having to deal with / at least explain this to contributors?

Still, would something like this be helpful? (or maybe just a set -x)

CODE=0
for patch in tools/example-showcase/*.patch; do
  git apply --ignore-whitespace $patch || { echo "::error::$patch failed to apply."; CODE=1; }
done
exit $CODE

(output using that code here: https://github.com/rparrett/bevy/actions/runs/8286681390/job/22677245376#step:7:14)

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Mar 17, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 17, 2024
@mockersf
Copy link
Member Author

But with this running on merge_group, I guess it's mostly maintainers having to deal with / at least explain this to contributors?

What worries me most is that git is not an easy tool to master, and I'm not sure how to write an error message that would teach anyone how to create a patch

Merged via the queue into bevyengine:main with commit 39385be Mar 17, 2024
27 checks passed
Comment on lines +306 to +307
- name: Installs cargo-udeps
run: cargo install --force cargo-udeps
Copy link
Member

Choose a reason for hiding this comment

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

I know I'm a bit late, but does this need to install cargo-udeps if we're just calling cargo build? Was this leftover from a copy-paste?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Build-System Related to build systems or continuous integration S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check example showcase patches in CI
6 participants