Fix ModuleThreadContext to only ignore dev dependencies when applicable#28957
Fix ModuleThreadContext to only ignore dev dependencies when applicable#28957namanONcode wants to merge 3 commits intobazelbuild:masterfrom
Conversation
Fixes #issue-21596 ModuleThreadContext.addOverride() had a blanket if (shouldIgnoreDevDeps()) return; that discarded all overrides when --ignore_dev_dependency was set, regardless of whether the overridden module was actually a dev dependency.
There was a problem hiding this comment.
Pull request overview
This PR refines Bzlmod’s handling of --ignore_dev_dependency by ensuring module overrides are only ignored for dev dependencies that are actually ignored, instead of dropping all overrides when the flag is set.
Changes:
- Track ignored dev-dependency module names during
bazel_dep(..., dev_dependency=True)when--ignore_dev_dependencyis enabled. - Filter overrides for those ignored dev dependencies when constructing the root module’s override map.
- Expand
ModuleFileFunctionTestcoverage to assert dev-only overrides are filtered while non-dev overrides remain.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java | Adds tracking of ignored dev deps and filters override output accordingly. |
| src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java | Records ignored dev deps when bazel_dep is skipped due to --ignore_dev_dependency. |
| src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java | Adds/adjusts tests asserting override filtering behavior with --ignore_dev_dependency. |
Comments suppressed due to low confidence (1)
src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java:363
addOverride()no longer ignores override directives in non-root modules. The Starlark docs for override directives state they only take effect in the root module; with this change, non-root module evaluation can now fail (e.g., duplicate overrides now throw) despite the overrides being semantically ignored. Consider short-circuiting here when the current module key is notModuleKey.ROOT, and keep the dev-dep filtering logic separate (only applied for the root module when--ignore_dev_dependencyis set).
public void addOverride(String moduleName, ModuleOverride override) throws EvalException {
ModuleOverride existingOverride = overrides.putIfAbsent(moduleName, override);
if (existingOverride != null) {
throw Starlark.errorf("multiple overrides for dep %s found", moduleName);
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java
Outdated
Show resolved
Hide resolved
|
Can you update the PR description according to our template? |
I Fixed and implemented all suggestion give a look |
|
I actually think the fix should be much simpler: if |
Closes #21596
Description
This pull request improves the handling of development-only dependencies (
dev_dependency) in Bazel's Bzlmod module system. Specifically, it ensures that any module overrides for dev-only dependencies are ignored when the--ignore_dev_dependencyflag is set. The changes introduce a mechanism to track ignored dev dependencies and filter out their overrides.Key changes:
Core logic improvements:
Set<String> ignoredDevDepstoModuleThreadContextto track module names of dev-only dependencies that are ignored when--ignore_dev_dependencyis enabled.buildOverridesmethod to remove any overrides for modules listed inignoredDevDeps, ensuring that overrides for ignored dev dependencies do not affect the build.bazelDeplogic to calladdIgnoredDevDepwhen a dev dependency is ignored, ensuring the ignored modules are properly tracked.Testing improvements:
ModuleFileFunctionTestto verify that overrides for dev-only dependencies are filtered out when--ignore_dev_dependencyis set, and that overrides for regular dependencies are preserved.Motivation
Fixes #21596
Previously,
ModuleThreadContext.addOverride()had a blanketif (shouldIgnoreDevDeps()) return;that discarded all overrides when--ignore_dev_dependencywas set, regardless of whether the overridden module was actually a dev dependency. This PR fixes this bug so that only the dev dependency overrides are safely ignored.Build API Changes
No
Checklist
Release Notes
RELNOTES: None