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

Added bootstrapping for crate_universe #663

Merged
merged 8 commits into from
Apr 7, 2021
Merged

Added bootstrapping for crate_universe #663

merged 8 commits into from
Apr 7, 2021

Conversation

UebelAndre
Copy link
Collaborator

@UebelAndre UebelAndre commented Mar 30, 2021

crate_universe_resolver is now built for various platforms when a commit containing changes to it is merged to main. The intent is to have users make changes to the code but never commit binaries, these binaries will then be built by CI added as an artifact to an automatic release which. This artifact will then be automatically wired up for use in the rules by committing a change to defaults.bzl back to the repo once the release URL and artifact sha256 are known.

Users looking to bootstrap crate_resolver locally for testing changes to rules_rust can run ./bootstrap.sh to have the repository configured to use a locally compiled binary.

A creates a release which looks like
Screen Shot 2021-04-03 at 5 19 13 PM

Followups:

  1. The bootstrapping script should be run before builds in Bazel CI to ensure an updated binary is always available.
  2. A check should be added to prevent any PR from containing an updated binary

@UebelAndre
Copy link
Collaborator Author

UebelAndre commented Mar 30, 2021

Once again foiled by the fact that the min target is no 4.0.0 (sh_binary.env)

Setting this up is extremely time consuming so I'd like to just get something in hopefully get some help improving this situation. For the time being, binaries can now be built by github actions and committed back to the repo (see here). In the absence of any place where the binaries can be hosted and the repo then kept up to date with the appropriate URLs and sha256 checksums, committing to the repo is the most effective way to make sure the binary is available and up to date.

@UebelAndre UebelAndre marked this pull request as ready for review March 30, 2021 16:38
@UebelAndre
Copy link
Collaborator Author

UebelAndre commented Mar 30, 2021

If we were able to use 4.0 then we could add

run_targets:
  - "//crate_universe/private:bootstrap"

but the again, the min target does not allow for how this is setup and there's just a path issue on windows, but otherwise it appears that the binaries can be "bootstrapped" and will run before the rest of the build. Maybe the run_targets list won't work long term in cases where a change was made to crate_universe and the examples updated in the same PR. This could probably be solved for by shell_commands to go and bootstrap the binaries outside of bazel or in some minimal workspace for use in the examples.

@UebelAndre UebelAndre marked this pull request as draft April 1, 2021 14:42
@google-cla
Copy link

google-cla bot commented Apr 1, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@google-cla
Copy link

google-cla bot commented Apr 1, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@google-cla
Copy link

google-cla bot commented Apr 1, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@google-cla
Copy link

google-cla bot commented Apr 1, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@google-cla
Copy link

google-cla bot commented Apr 1, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Apr 1, 2021
@UebelAndre UebelAndre marked this pull request as ready for review April 4, 2021 00:20
Copy link
Collaborator

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

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

This is looking great, thanks for taking the time to put it together! I think the approach generally looks good - would you mind adding a quick README.md, maybe in crate_universe/private/bootstrap, explaining the general flow, and how the pieces fit together?

To make sure I understand the assorted workflows:

  • For local development, I should run ./bootstrap.sh every time I change crate_universe to see its effects - if I don't, I'll just use stale binaries (potentially stale from a previous build - maybe we should print something to the console if you're using an overridden binary? I'm imagining people doing a build, having the .bazelrc override files in place, forgetting about them, and then getting confused for future builds...).
  • When changes to crate_universe are merged, main will automatically be updated in a separate commit so that binaries are downloaded from GitHub releases.
  • For PRs, right now crate_universe stuff won't work (which is fine), but the idea for the future is that each presubmit shard will (effectively, maybe not literally) run ./bootstrap.sh before running so that it has locally up-to-date binaries reflecting any changes.

Is that all correct?

.github/workflows/crate_universe.yaml Outdated Show resolved Hide resolved
.github/workflows/crate_universe.yaml Outdated Show resolved Hide resolved
rust/repositories.bzl Show resolved Hide resolved
.github/workflows/crate_universe.yaml Outdated Show resolved Hide resolved
.github/workflows/crate_universe.yaml Outdated Show resolved Hide resolved
crate_universe/private/bootstrap/build.sh Outdated Show resolved Hide resolved
crate_universe/private/bootstrap/build.sh Outdated Show resolved Hide resolved
crate_universe/private/bootstrap/install.sh Outdated Show resolved Hide resolved
@@ -0,0 +1,14 @@
"""A helper module defining generated information about crate_universe dependencies"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this file be deleted? It feels like this should only ever contain actual sha256 values?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's very likely a smarter way to do this but this was easiest so would like to defer additional improvements here to another PR unless you feel strongly.

# Only bootstrap the binary if there are changes
if [[ -n "${has_changes}" ]]; then
pushd "${BOOTSTRAP_DIR}" &> /dev/null
bazel run //:build && bazel run //:install && bazel clean
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why the bazel clean?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So we avoid recursive symlinks when using override_repository. The clean should be fast since the only thing built by bazel is a shell script.

@illicitonion
Copy link
Collaborator

FWIW I think this is a reasonable justification to bump our minimum bazel version to 4.0.0 (before we make PR builds work)

@UebelAndre
Copy link
Collaborator Author

* For local development, I should run `./bootstrap.sh` every time I change `crate_universe` to see its effects - if I don't, I'll just use stale binaries (potentially stale from a previous build - maybe we should print something to the console if you're using an overridden binary? I'm imagining people doing a build, having the .bazelrc override files in place, forgetting about them, and then getting confused for future builds...).

Yes, you should run ./bootstrap.sh for every change you make to the resolver so they can be tested locally in the examples. If we're concerned about future contributors forgetting about the .bazelrc file then we could do something to improve the process. But I'd rather defer that to another PR. I've added announce_rc for the time being to act as a reminder that the overrides are in use.

* When changes to `crate_universe` are merged, `main` will automatically be updated in a separate commit so that binaries are downloaded from GitHub releases.

Correct, If any change is made to main that contains changes to crate_universe/** then new binaries will be built and associated with an auto-created pre-release.

* For PRs, right now `crate_universe` stuff won't work (which is fine), but the idea for the future is that each presubmit shard will (effectively, maybe not literally) run `./bootstrap.sh` before running so that it has locally up-to-date binaries reflecting any changes.

correct, we cannot rely on crate_universe right now until we have binaries in place or bootstrapping enabled (either by calling the script directly or doing something to a similar end).

@UebelAndre UebelAndre requested a review from illicitonion April 7, 2021 15:37
@google-cla
Copy link

google-cla bot commented Apr 7, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Apr 7, 2021
@google-cla
Copy link

google-cla bot commented Apr 7, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

Co-authored-by: Daniel Wagner-Hall <[email protected]>
@google-cla
Copy link

google-cla bot commented Apr 7, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

3 similar comments
@google-cla
Copy link

google-cla bot commented Apr 7, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla
Copy link

google-cla bot commented Apr 7, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla
Copy link

google-cla bot commented Apr 7, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla
Copy link

google-cla bot commented Apr 7, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

Copy link
Collaborator

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

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

Works for me - thanks!!

@illicitonion
Copy link
Collaborator

@googlebot I consent.

@google-cla google-cla bot added cla: yes and removed cla: no labels Apr 7, 2021
@UebelAndre UebelAndre merged commit a3c2741 into bazelbuild:main Apr 7, 2021
@UebelAndre UebelAndre deleted the universe branch April 7, 2021 17:26
yagehu pushed a commit to yagehu/rules_rust that referenced this pull request Apr 23, 2021
* Added bootstrap script for crate_universe and github action for auto-releasing new binaries

* Addressed PR feedback

Co-authored-by: Daniel Wagner-Hall <[email protected]>

* Updated local dev documentation

* Add announce_rc to crate_universe.bazelrc

* Addressed buildifier defect

* Addressed additional user feedback

* Apply suggestions from code review

Co-authored-by: Daniel Wagner-Hall <[email protected]>

* Fixed missing extension for windows.

Co-authored-by: Daniel Wagner-Hall <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants