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

Streamline CI #17224

Merged
merged 7 commits into from
Oct 14, 2022
Merged

Streamline CI #17224

merged 7 commits into from
Oct 14, 2022

Conversation

benjyw
Copy link
Contributor

@benjyw benjyw commented Oct 14, 2022

  • Unifies the code that generates the "Build wheels" jobs, so they are now uniform (with allowances for the containerization on Linux). This means in particular that we no longer bootstrap Pants on macOS11 on ARM64, as there was no particular reason to, and we weren't doing so on macOS10.15 on x86 for example.
  • Uses the new classifier to skip these jobs entirely if unneeded.
  • Similarly, uses the new classifier to skip rust steps if unneeded.
  • Removes the steps for getting the commit message, since we no longer need it.

Now, to force a rust or wheel build even if no files were affected, you can temporarily modify a relevant file. If this becomes a hassle we can think of another way to achieve this, that doesn't involve the commit message.

@benjyw benjyw added the category:internal CI, fixes for not-yet-released features, etc. label Oct 14, 2022
@benjyw benjyw marked this pull request as draft October 14, 2022 05:50
@benjyw benjyw force-pushed the streamline_ci branch 2 times, most recently from e0e42b7 to 6e88bbe Compare October 14, 2022 07:02
@benjyw benjyw requested a review from stuhood October 14, 2022 07:14
@benjyw
Copy link
Contributor Author

benjyw commented Oct 14, 2022

Still need to figure out why the "Set Merge OK" job didn't run. It depends on skipped jobs, but the github docs say "Note: A job that is skipped will report its status as "Success". It will not prevent a pull request from merging, even if it is a required check." : https://docs.github.com/en/actions/using-jobs/using-conditions-to-control-job-execution

@benjyw
Copy link
Contributor Author

benjyw commented Oct 14, 2022

Annoyingly, a skipped job counts as successful for the purpose of branch protection, but jobs that depend on it still don't run. Working around this now. The workaround might actually allow us to simplify the merge-ok dance.

@benjyw benjyw requested a review from thejcannon October 14, 2022 17:52
@benjyw benjyw marked this pull request as ready for review October 14, 2022 17:53
Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

"github.event_name == 'push' || !contains(env.COMMIT_MESSAGE, '[ci skip-build-wheels]')"
)

DONT_SKIP_RUST = "needs.classify_changes.outputs.rust == true"
Copy link
Member

Choose a reason for hiding this comment

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

Oh. Hm... I just realized that your classification changes made

# Ensure that this stays in sync with `build-support/bin/rust/calculate_engine_hash.sh`.
NUM_RUST_FILES=$(echo "${CHANGED_FILES})" | grep -c -E \
-e "^src/rust/engine" \
-e "^rust-toolchain" \
-e "^build-support/bin/rust" \
-e "^build-support/bin/generate_github_workflows.py")
NUM_RELEASE_FILES=$(echo "${CHANGED_FILES})" | grep -c -E \
-e "^pants.toml" \
-e "^src/python/pants/__init__" \
-e "^src/python/pants/VERSION" \
-e "^src/python/pants/notes" \
-e "^src/python/pants/init/BUILD" \
-e "^src/rust/engine/fs/fs_util" \
-e "^build-support/bin/release.sh" \
-e "^build-support/bin/release_helper.py" \
-e "^build-support/bin/generate_github_workflows.py")
# To avoid putting skip labels multiple times, check if the labels already exist
# in the commit message.
grep "\[ci skip-rust\]" "${COMMIT_MSG_FILEPATH}" > /dev/null
HAS_RUST_SKIP=$?
grep "\[ci skip-build-wheels\]" "${COMMIT_MSG_FILEPATH}" > /dev/null
HAS_WHEELS_SKIP=$?
obsolete?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! Planning to remove those in a followup.

# For manylinux compatibility, we build wheels in a container rather than directly
# on the Ubuntu runner. As a result, we have custom steps here to check out
# the code, install rustup and expose Pythons.
# TODO: Run tests etc. using this container image, so we can reuse the rust engine build?
Copy link
Member

Choose a reason for hiding this comment

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

Tests run in debug mode by default, so they won't share any compilation with production/release wheel builds. The only time you would have overlap would be in PR runs that affected wheel builds... probably not worth it to try and share between tests and release builds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, will adjust the comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But we could still profitably cache cargo etc presumably?

Copy link
Member

Choose a reason for hiding this comment

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

Possibly, yea.

@benjyw benjyw merged commit 89ef823 into pantsbuild:main Oct 14, 2022
@benjyw benjyw deleted the streamline_ci branch October 14, 2022 23:58
benjyw added a commit to benjyw/pants that referenced this pull request Oct 15, 2022
- Unifies the code that generates the "Build wheels" jobs, so they are now uniform (with allowances for the containerization on Linux). This means in particular that we no longer bootstrap Pants on macOS11 on ARM64, as there was no particular reason to, and we weren't doing so on macOS10.15 on x86 for example.
- Uses the new classifier to skip these jobs entirely if unneeded.
- Similarly, uses the new classifier to skip rust steps if unneeded.
- Removes the steps for getting the commit message, since we no longer need it.

Now, to force a rust or wheel build even if no files were affected, you can temporarily modify a relevant file. If this becomes a hassle we can think of another way to achieve this, that doesn't involve the commit message.
benjyw added a commit that referenced this pull request Oct 16, 2022
- Unifies the code that generates the "Build wheels" jobs, so they are now uniform (with allowances for the containerization on Linux). This means in particular that we no longer bootstrap Pants on macOS11 on ARM64, as there was no particular reason to, and we weren't doing so on macOS10.15 on x86 for example.
- Uses the new classifier to skip these jobs entirely if unneeded.
- Similarly, uses the new classifier to skip rust steps if unneeded.
- Removes the steps for getting the commit message, since we no longer need it.

Now, to force a rust or wheel build even if no files were affected, you can temporarily modify a relevant file. If this becomes a hassle we can think of another way to achieve this, that doesn't involve the commit message.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:internal CI, fixes for not-yet-released features, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants