Add --inject_repository and fix crash on overridden non-existent repo
#23791
Conversation
fmeum
requested review from
a team,
lberki,
Wyverald and
meteorcloudy
as code owners
fmeum
requested review from
katre
and removed request for
a team,
lberki and
katre
github-actions
bot
added
team-Configurability
| if (repositoryName.isMain()) { | ||
| repoMapping = | ||
| repoMapping.withAdditionalMappings( | ||
| repositoryOverrides.stream().collect(toMap(RepositoryName::getName, name -> name))); |
There was a problem hiding this comment.
This is a bit unfortunate as the resulting repo will have a canonical repo name that is also a valid apparent name. We could prevent that by branching on whether the name provided on the command line is a valid apparent name or not:
- if it isn't, but is a valid canonical name, assume that it overrides an existing repo
- if it is, generate a synthetic canonical repo name such as
_main+_overrides+<name>for it.
What do you think? Would the resulting consistency be worth the increased complexity?
There was a problem hiding this comment.
(tests are failing because REPOSITORY_OVERRIDES would now need to be injected in more tests, I can do that once we have agreed on the functionality)
There was a problem hiding this comment.
could we maybe call the flag --inject_repository, similar to inject_repo vs override_repo? There's a nice bit of symmetry there, at least superficially... Then we could also say --inject_repository=X=foo always creates the repo with the canonical repo name of +_injected+X, or something.
There was a problem hiding this comment.
I completely rewrote this PR. It now implements --inject_repository through a simulated use_repo_rule call for a local_repository, which guarantees behavior identical to that of a regular module extension repo.
sgowroji
added
awaiting-user-response
fmeum
force-pushed
the
22691-override-repository
branch
from
711ef17
to
273012d
Compare
RELNOTES: The new `--inject_repository` flag can be used to add new repositories via the CLI with `--noenable_workspace`. Such repositories behave as if they were declared by `local_repository` via `use_repo_rule` in the root module.
fmeum
force-pushed
the
22691-override-repository
branch
from
273012d
to
1d97bcb
Compare
fmeum
changed the title
--override_repository to add repos with --noenable_workspace
fmeum
changed the title
--inject_repository and fix crash on overridden non-existent repo
|
There are a number of test failures referencing files under |
Wyverald
added
awaiting-review
| @@ -95,6 +97,8 @@ public class ModuleFileFunction implements SkyFunction { | |||
|
|
|||
| public static final Precomputed<Map<String, ModuleOverride>> MODULE_OVERRIDES = | |||
| new Precomputed<>("module_overrides"); | |||
| public static final Precomputed<Map<String, PathFragment>> INJECTED_REPOSITORIES = | |||
There was a problem hiding this comment.
let's use ImmutableMap everywhere (unless the values can be null, which doesn't seem to be the case)
| new ModuleFileGlobals.ModuleExtensionProxy( | ||
| usageBuilder, | ||
| ModuleExtensionUsage.Proxy.builder() | ||
| .setDevDependency(true) |
There was a problem hiding this comment.
does this matter at all? just checking. If not, I have a slight preference for not setting it for least surprise, but not a big deal.
| + " corresponding `local_repository` to the root module's MODULE.bazel file via" | ||
| + " `use_repo_rule`. If the given path is an absolute path, it will be used as it is." | ||
| + " If the given path is a relative path, it is relative to the current working" | ||
| + " directory. If the given path starts with '%workspace%, it is relative to the" |
There was a problem hiding this comment.
| + " directory. If the given path starts with '%workspace%, it is relative to the" | |
| + " directory. If the given path starts with '%workspace%', it is relative to the" |
| + " If the given path is a relative path, it is relative to the current working" | ||
| + " directory. If the given path starts with '%workspace%, it is relative to the" | ||
| + " workspace root, which is the output of `bazel info workspace`. If the given path" | ||
| + " is empty, then remove any previous overrides.") |
There was a problem hiding this comment.
| + " is empty, then remove any previous overrides.") | |
| + " is empty, then remove any previous injections.") |
|
|
||
| public abstract String path(); | ||
| } | ||
| public record RepositoryInjection(String apparentName, String path) {} |
| } | ||
| if (repositoryMappingValue == RepositoryMappingValue.NOT_FOUND_VALUE | ||
| && REPOSITORY_OVERRIDES.get(env).containsKey(key.repoName())) { | ||
| throw new RepositoryMappingFunctionException( |
Suggest the new flag instead of crashing when
--override_repositoryis applied to a non-existent repo with--noenable_workspace.Fixes #22691
RELNOTES: The new
--inject_repositoryflag can be used to add new repositories via the CLI with--enable_bzlmod. Such repositories behave as if they were declared bylocal_repositoryviause_repo_rulein the root module.