-
Notifications
You must be signed in to change notification settings - Fork 15
Add bzlmod support for arch_alias with module extension #3131
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for nifty-bassi-e26446 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
3c3973f to
c51ca8f
Compare
Signed-off-by: Matthieu MOREL <matthieu.morel35@gmail.com>
c51ca8f to
7529bd1
Compare
phlax
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for working on this @mmorel-35
can you separate all the test changes out to a different pr please - lets make the change and then consider if/where to test as an external repo
(and remove all the bot docs - not against adding docs but they need to be minimal, focused and targeted and easy to maintain)
| @@ -0,0 +1,11 @@ | |||
| # Bazel configuration for arch_alias extension test module | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the test files here seem misplaced - altho i understand these tests need a separate workspace to use toolshed externally, which i think is not a bad idea - but then it should be testing non-bzlmod also
| "amd64": "@platforms//host", | ||
| "aarch64": "@platforms//host", | ||
| "x86_64": "@platforms//host", | ||
| "arm64": "@platforms//host", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure iiuc but what is the point of the test if they all return the same thing
| @@ -0,0 +1,32 @@ | |||
| # arch_alias Tests | |||
|
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please dont keep submitting voluminous bot docs - it overwhelms reviewers and detracts from the actual changes
Fix arch_alias compatibility with bzlmod
clang_platform is not compatible with bzlmod: https://github.com/envoyproxy/envoy/actions/runs/20279190616/job/58236274568
Analysis
The
arch_aliasrepository rule inbazel/repository/utils.bzlwas created to enable architecture-specific platform aliases for Envoy. It works by:.bazelrcto reference platform aliases like@clang_platformProblem
The current implementation is WORKSPACE-only and doesn't follow bzlmod best practices:
--enable_bzlmodis setSolution Implemented
arch_alias/and remove redundant_testsuffixalias_nameparameter - the logic automatically handles bzlmod canonical namesChanges Made
arch_alias_extmodule extension with proper tag_class_get_alias_name()helper automatically extracts apparent names from bzlmod's canonical format (module++ext+name→name)alias_nameparameter as it was unnecessarybazel/test/arch_alias/bzlmod/- BCR-compatible bzlmod test modulebazel/test/arch_alias/workspace/- WORKSPACE mode backward compatibility tests_testsuffix.github/workflows/_bazel.yml) for both bzlmod and workspace modestest_arch_aliastask to BCR presubmit configuration (.bcr/bazel/presubmit.yml)module()declaration from test MODULE.bazel as it's not intended for publicationTesting
All tests pass in both bzlmod and WORKSPACE modes, verifying multiple aliases work correctly. Tests now run automatically in CI/CD pipelines on every PR and in BCR presubmit checks.