Skip to content

crate_universe: Render manifests with starlark#1425

Open
uhthomas wants to merge 1 commit intobazelbuild:mainfrom
uhthomas:1287
Open

crate_universe: Render manifests with starlark#1425
uhthomas wants to merge 1 commit intobazelbuild:mainfrom
uhthomas:1287

Conversation

@uhthomas
Copy link
Copy Markdown
Contributor

Fixes #1287

@UebelAndre UebelAndre self-requested a review June 30, 2022 13:27
Copy link
Copy Markdown
Collaborator

@UebelAndre UebelAndre left a comment

Choose a reason for hiding this comment

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

Hey, I haven't done a real pass through this code yet but did see a ton of deletion I didn't expect. Why wouldn't this change initally be a new crates_vendor mode that writes the "Bazel lock files" and uses the new functionality here to render it into repository rules? It seems far less invasive and give folks a chance to easily transition their setups.

@uhthomas
Copy link
Copy Markdown
Contributor Author

uhthomas commented Jul 6, 2022

Hey, I haven't done a real pass through this code yet but did see a ton of deletion I didn't expect. Why wouldn't this change initally be a new crates_vendor mode that writes the "Bazel lock files" and uses the new functionality here to render it into repository rules? It seems far less invasive and give folks a chance to easily transition their setups.

Hmm, I suppose it makes sense to keep the existing functionally in addition to this?

The approach taken in this change differs dramatically as the build files aren't rendered, but instead interpreted which makes it fundamentally incompatible with the existing approach. Maintaining both would be tedious and strange.

I'm not at a computer at the minute so I can't give an example; the build files for the repository rules are almost all identical. They contain a snippet of the Cargo Bazel manifest and call a macro to interpret said snippet. Vendoring these would be pointless.

I mentioned this in the original issue: it is possible to see the result of the macros with bazel query --output=build @crate_universe_crate_index//... or bazel query --output=build @crate_universe_crate_index__toml-0.5.9//.... Might be helpful?

Happy to discuss and make changes where necessary :) Some work still needs to be done for feature parity.

@UebelAndre
Copy link
Copy Markdown
Collaborator

The approach taken in this change differs dramatically as the build files aren't rendered, but instead interpreted which makes it fundamentally incompatible with the existing approach. Maintaining both would be tedious and strange.

Perhaps I don't fully understand the changes being made. My current understanding of the fundamental change is this is a new set of functionality that could be added along side the existing functionality and things be deleted in a second step (making the initial implementation is less noisy). Could you link some updated docs that demonstrate the new workflow?

@uhthomas
Copy link
Copy Markdown
Contributor Author

uhthomas commented Jul 6, 2022

The approach taken in this change differs dramatically as the build files aren't rendered, but instead interpreted which makes it fundamentally incompatible with the existing approach. Maintaining both would be tedious and strange.

Perhaps I don't fully understand the changes being made. My current understanding of the fundamental change is this is a new set of functionality that could be added along side the existing functionality and things be deleted in a second step (making the initial implementation is less noisy). Could you link some updated docs that demonstrate the new workflow?

I'm on holiday this week unfortunately, though I would be happy to write some documentation when I'm back. The workflow should be almost identical, aside from bazel run @crate_universe_crate_index//:repin to repin instead of bazel sync.

I believe I wrote it this way because the cargo-bazel binary has been completely removed from the analysis phase. I guess it may be possible to write a new repository rule for this functionality instead of rewriting the old one?

@UebelAndre
Copy link
Copy Markdown
Collaborator

I'm on holiday this week unfortunately

Nothing unfortunate about being on holiday, enjoy the time off 😄.

I think this change needs more design discussion but understand the desire to remove the need for the cargo-bazel binary in the analysis phase. When you're back we can continue the conversation.

@uhthomas
Copy link
Copy Markdown
Contributor Author

I'm back from holiday. Would you like to have a design discussion? It would be good to understand the problems and limitations with current system, what an ideal system would look like and how to get there.

Would it make more sense to build something based around bzlmod? There are some examples of how rules_js, rules_go/gazelle and rules_python are doing this:

side note: Need to considers forwarding environment variables as some git repositories may depend on ssh which may in turn depend on $PATH or $SSH_AUTH_SOCK.

@UebelAndre
Copy link
Copy Markdown
Collaborator

My expectation for this feature is to:

  1. Add a new mode to crates_vendor, lock
  2. Implement functionality for rendering BUILD files from the json file that represents the lock file that would now be output by crates_vendor.
  3. Update the generated the crates module template to instead use the new rendering functionality to generate remote repository rules.

I think this is the most idiomatic form of what you're asking for. Updating/repinning is now an explicit step that does not happen in the analysis phase and the footprint of these dependencies are limited to just one file.

How does that sound?

@mering
Copy link
Copy Markdown

mering commented May 15, 2024

@uhthomas @UebelAndre Has there been any progress outside of this PR in the past two years?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

crate_universe: Rewrite generator in starlark

3 participants