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

Fix shell commands in bcr testing #3070

Merged
merged 4 commits into from
Dec 10, 2024
Merged

Conversation

UebelAndre
Copy link
Collaborator

No description provided.

@UebelAndre UebelAndre marked this pull request as ready for review December 9, 2024 15:41
@UebelAndre UebelAndre force-pushed the releases branch 7 times, most recently from 8c25ebe to b9944ca Compare December 9, 2024 17:37
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.

There's probably scope for tidying this up (e.g. referencing some variables, rather than copy+pasting the lines), but also, I don't think we super need to worry about maintenance here, it's really just Yet Another edit to make on release :D

Thank you, as ever, for all the digging and fixing!

@UebelAndre
Copy link
Collaborator Author

There's probably scope for tidying this up (e.g. referencing some variables, rather than copy+pasting the lines), but also, I don't think we super need to worry about maintenance here, it's really just Yet Another edit to make on release :D

Thank you, as ever, for all the digging and fixing!

Yeah, going through all this I wonder if I shouldn't just call it quits and publish everything within a single rules_rust archive but I think an ideal world is for each extension to be in it's own archive and so I work toward that end. I'll need to document what it takes to add a module and clean all that up. The release.yaml workflow is currently a mess and the infra is otherwise pretty brittle and painful to test. One day though!

@UebelAndre UebelAndre force-pushed the releases branch 5 times, most recently from 3eb87c9 to eb80af5 Compare December 10, 2024 07:30
@UebelAndre
Copy link
Collaborator Author

I'm struggling to come up with a way to download the new rules_rust archive in CI for --override_module. I'm wondering if I'm hitting some intentional security limitations. @meteorcloudy I'm wondering if you can provide some insight.

@meteorcloudy
Copy link
Member

everything within a single rules_rust archive

I think this might be a better idea, with the repository cache, the archive will only be downloaded by Bazel for once.

@UebelAndre
Copy link
Collaborator Author

everything within a single rules_rust archive

I think this might be a better idea, with the repository cache, the archive will only be downloaded by Bazel for once.

Is that such a huge deal with such tiny archives? I think I would prefer the extra download for faster downloads and extracts of just the modules I care about.

@meteorcloudy
Copy link
Member

I thought those smaller modules all depend on rules_rust already? Indeed the extract time might be faster.

Anyway, can you explain a bit why you need to download those archives in the presubmit.yml file?

@UebelAndre UebelAndre force-pushed the releases branch 2 times, most recently from 7b9964a to 12f7d19 Compare December 10, 2024 14:40
@UebelAndre
Copy link
Collaborator Author

Anyway, can you explain a bit why you need to download those archives in the presubmit.yml file?

Looking at the failure here https://buildkite.com/bazel/bcr-presubmit/builds/9256#0193ac0a-f993-42ec-9d44-747732373250 (bazelbuild/bazel-central-registry#3359), the module is trying to use a directory outside the checkout as rules_rust. I want to have submodules in rules_rust be bzlmod modules backed by their own archives so the MODULE.bazel file needs to work independently. It would seem using local_path_override actually seems to prevent that entirely so I'm trying to instead use --override_module and some gymnastics to download core rules_rust and extract it for use in the bcr tests. Ideally there would be a section in the BCR yaml that lets me pair a module name with a url and does this for me so repositories which publish multiple archives can properly test them without needing to bundle monolithic modules that contain tons of unnecessary/unused stuff by consumers.

@meteorcloudy
Copy link
Member

Have you tried archive_override or git_override?

@UebelAndre
Copy link
Collaborator Author

Have you tried archive_override or git_override?

Why is that any different than local_path_override? If I put this in MODULE.bazel then doesn't it impact consumers? If I make it a dev dependency then tests will fail because it can't find the current version. Feels like lose-lose.

@meteorcloudy
Copy link
Member

overrides won't propagate to dependent, you can just keep them in the MODULE.bazel file, they will be ignored when the module is used as a dependency. Would that work?

@UebelAndre UebelAndre added this pull request to the merge queue Dec 10, 2024
@UebelAndre
Copy link
Collaborator Author

overrides won't propagate to dependent, you can just keep them in the MODULE.bazel file, they will be ignored when the module is used as a dependency. Would that work?

Ah, that's good to know, and that might have worked in combination with some sed hackery. But I instead consolidated everything into a single archive. The round trip of changes to the repo to BCR CI is quite slow and I really wanna move on from this.

Merged via the queue into bazelbuild:main with commit dcf0a57 Dec 10, 2024
4 checks passed
@UebelAndre UebelAndre mentioned this pull request Dec 10, 2024
github-merge-queue bot pushed a commit that referenced this pull request Dec 10, 2024
This was a regression from
#3070
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.

3 participants