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

Allow --override_repository to accept apparent repo names (as seen from the main repo), in addition to canonical repo names #17128

Open
keith opened this issue Jan 3, 2023 · 9 comments
Assignees
Labels
area-Bzlmod Bzlmod-specific PRs, issues, and feature requests not stale Issues or PRs that are inactive but not considered stale P2 We'll consider working on this in future. (Assignee optional) team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. type: feature request

Comments

@keith
Copy link
Member

keith commented Jan 3, 2023

Description of the bug:

If you have a module extension with bzlmod that loads arbitrary http_archives like:

use_repo(
    deps,
    "Yams",
...

you cannot override it with --override_module because of the capital letter:

--override_module=Yams=/Users/ksmiley/dev/Yams:

ERROR: While parsing option --override_module=Yams=/Users/ksmiley/dev/Yams: invalid module name 'Yams': valid names must 1) only contain lowercase letters (a-z), digits (0-9), dots (.), hyphens (-), and underscores (_); 2) begin with a lowercase letter; 3) end with a lowercase letter or digit.

Using ---override_repository also has no affect.

I'm not sure if the bug here is that these names should be allowed, or if there should be a different way to override these

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

No response

Which operating system are you running Bazel on?

No response

What is the output of bazel info release?

6.0.0rc4

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

No response

What's the output of git remote get-url origin; git rev-parse master; git rev-parse HEAD ?

No response

Have you found anything relevant by searching the web?

No response

Any other information, logs, or outputs that you want to share?

No response

@keith
Copy link
Member Author

keith commented Jan 3, 2023

This also doesn't work if you use local_path_override

@keith
Copy link
Member Author

keith commented Jan 3, 2023

in a quick test just allowing uppercase module names also doesn't work, but i imagine they could be being transformed elsewhere as well.

@Wyverald
Copy link
Member

Wyverald commented Jan 3, 2023

"Yams" here isn't a module -- it's not from a registry, doesn't participate in version resolution, etc. You can use --override_repository, but with the canonical repo name only (_main~$EXTENSION_NAME~Yams)

@keith
Copy link
Member Author

keith commented Jan 3, 2023

Good to have that workaround. Is this something you plan to improve?

@Wyverald
Copy link
Member

Wyverald commented Jan 3, 2023

Hmm... I think one way to improve this might be to let --override_repository accept keys that start with either @ or @@, similar to how labels are specified on the command line. So --override_repository=_main~$EXTENSION_NAME~Yams=dir is equivalent to --override_repository=@@_main~$EXTENSION_NAME~Yams=dir or --override_repository=@Yams=dir (the singular @ makes it go through the main repo mapping).

@sgowroji sgowroji added type: bug team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. untriaged area-Bzlmod Bzlmod-specific PRs, issues, and feature requests labels Jan 4, 2023
@Wyverald Wyverald added type: feature request P2 We'll consider working on this in future. (Assignee optional) and removed type: bug untriaged labels Jan 4, 2023
@Wyverald Wyverald changed the title 'invalid module name' with --override_module and module extensions Allow --override_repository to accept apparent repo names (as seen from the main repo), in addition to canonical repo names Jan 4, 2023
@github-actions
Copy link

github-actions bot commented Feb 9, 2023

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 30 days. It will be closed in the next 7 days unless any other activity occurs or the "not stale" label is added.

@github-actions github-actions bot added the stale Issues or PRs that are stale (no activity for 30 days) label Feb 9, 2023
@brentleyjones
Copy link
Contributor

@keertk not stale, triaged and not waiting for input

@sgowroji sgowroji removed the stale Issues or PRs that are stale (no activity for 30 days) label Feb 9, 2023
@keertk keertk added the not stale Issues or PRs that are inactive but not considered stale label Feb 9, 2023
binarin added a commit to rabbitmq/osiris that referenced this issue Apr 19, 2023
Until
bazelbuild/bazel#17128 (comment)
is implemented, there is no clean way to override an erlang
package. So at least let's try doing our best, and just check
afterwards that it actually worked.
binarin added a commit to rabbitmq/osiris that referenced this issue Apr 19, 2023
Until
bazelbuild/bazel#17128 (comment)
is implemented, there is no clean way to override an erlang
package. So at least let's try doing our best, and just check
afterwards that it actually worked.
binarin added a commit to rabbitmq/osiris that referenced this issue Apr 19, 2023
Until
bazelbuild/bazel#17128 (comment)
is implemented, there is no clean way to override an erlang
package. So at least let's try doing our best, and just check
afterwards that it actually worked.
@meteorcloudy meteorcloudy added this to the 7.0.0 branch cut milestone Sep 7, 2023
@iancha1992 iancha1992 removed this from the 7.0.0 release blockers milestone Oct 18, 2023
@iancha1992
Copy link
Member

@bazel-io fork 7.0.0

@keertk keertk added the P1 I'll work on this now. (Assignee required) label Nov 9, 2023
@Wyverald Wyverald self-assigned this Nov 9, 2023
@Wyverald
Copy link
Member

Wyverald commented Nov 9, 2023

After taking a brief look, I realized that this is actually surprisingly annoying to implement. This would be a rare flag that influences repo fetching / Bzlmod resolution, but also relies on the result of repo fetching / Bzlmod resolution (calculating the main repo mapping might require 'fetching' bazel_tools, for example).

We could make --override_repository one of those flags that get re-parsed after main repo mapping is computed, but this has multiple downsides: 1) the main repo mapping is only computed for commands that "build", which excludes fetch, sync, etc -- whose behaviors should be affected by --override_repository; 2) the option re-parsing step actually happens way after BlazeModule#beforeCommand, but all the repo options are initialized in BazelRepositoryModule#beforeCommand.

Alternatively, we could somehow remember that some apparent repo names were passed to --override_repository, and only resolve them in RepositoryDelegatorFunction. The problem is, though, that RDF only knows about the canonical repo name; it's potentially possible for RDF to request the main repo mapping, but that's incredibly convoluted and things are very likely to go wrong if we do it this way.

So I'm going to remove this from the 7.0 blockers and maybe tackle it in the future instead.

@Wyverald Wyverald added P2 We'll consider working on this in future. (Assignee optional) and removed P1 I'll work on this now. (Assignee required) labels Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Bzlmod Bzlmod-specific PRs, issues, and feature requests not stale Issues or PRs that are inactive but not considered stale P2 We'll consider working on this in future. (Assignee optional) team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. type: feature request
Projects
Status: Todo
Development

No branches or pull requests

7 participants