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

Pass command-line flag values through repo mapping #14852

Closed
lukokr-aarch64 opened this issue Feb 17, 2022 · 2 comments
Closed

Pass command-line flag values through repo mapping #14852

lukokr-aarch64 opened this issue Feb 17, 2022 · 2 comments
Assignees
Labels
area-Bzlmod Bzlmod-specific PRs, issues, and feature requests P2 We'll consider working on this in future. (Assignee optional) team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. type: bug

Comments

@lukokr-aarch64
Copy link

Description of the problem / feature request:

Hello, I am currently working on a Bazel module to store platforms used by our project. I import these using

bazel_dep(name = "myplatforms", version = "1.0.0-alpha.1")

And in my example binary I have something like:

cc_binary(
    name = "hello-world",
    srcs = ["hello-world.cc"],
    local_defines = select({
        "@myplatforms//<specific constraint>": ["<some define>=1"],
        "//conditions:default": [],
    })
)

✅ I can build all targets in the platform module:

bazelisk build @myplatforms//...
(14:11:51) INFO: Build completed successfully, 1 total action  

✅ I can also build all targets in the project:

bazelisk build //...
(14:17:43) INFO: Build completed successfully, 6 total actions

⚠️ But when I try to specify a platform things get a little weird:

bazelisk build //... --platforms @myplatforms//platform:<myplatform>
(14:19:59) ERROR: <path>/bazel-cpp-example/BUILD:1:10: While resolving toolchains for target //:srcs: com.google.devtools.build.lib.packages.BuildFileNotFoundException: no such package '@myplatforms//platform': The repository '@myplatforms' could not be resolved: Repository '@myplatforms' is not defined

✅ So it seems I need to pass in the exact module name with version:

bazelisk build //... --platforms @myplatforms.1.0.0-alpha.1//platform:<myplatform>
(14:21:00) INFO: Build completed successfully, 1 total action

It would be a nice quality of life feature if it wasn't needed to specify the full . string for the platforms module since it is already declared in MODULE.bazel.

Bazel SHA: eeec121
Also tried with 5.0.0

Feature requests: what underlying problem are you trying to solve with this feature?

Automatically resolve repositories for external platforms.

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

To reproduce you will need to self host a registry with a module containing the platform you would like to use. Then declare that module in your project and try to pass it to the build as shown in the description above.

What operating system are you running Bazel on?

❯ lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description:    Ubuntu 20.04.3 LTS
Release:        20.04
Codename:       focal

What's the output of bazel info release?

release 5.0.0

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

NA

Have you found anything relevant by searching the web?

I have not managed to find examples of builds using platforms from modules. Maybe that would be a useful addition.

@Wyverald Wyverald self-assigned this Feb 17, 2022
@Wyverald Wyverald added area-Bzlmod Bzlmod-specific PRs, issues, and feature requests P1 I'll work on this now. (Assignee required) team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. type: bug labels Feb 17, 2022
@Wyverald
Copy link
Member

The main issue here is that command-line flag values don't go through repo mapping. (Even though top-level build/test target patterns already do)

@meteorcloudy meteorcloudy added P2 We'll consider working on this in future. (Assignee optional) area-Bzlmod Bzlmod-specific PRs, issues, and feature requests and removed P1 I'll work on this now. (Assignee required) area-Bzlmod Bzlmod-specific PRs, issues, and feature requests labels Feb 21, 2022
@aiuto
Copy link
Contributor

aiuto commented Apr 28, 2022

We're going to have to fix that. All the rule sets will eventually define their own command line flags, and we need them to resolve to the right repository. We also need those rules to provide a way to say that there can only be one version after dependency resolution. It does no good if we end up with 2 versions of rules_java and you set a flag that it defines, but it only impacts one of those.

@Wyverald Wyverald changed the title Resolve repository mappings for --platforms Pass command-line flag values through repo mapping May 24, 2022
copybara-service bot pushed a commit that referenced this issue Jun 14, 2022
*** Reason for rollback ***

Temporarily rolling back until #14852 is fixed.

*** Original change description ***

Use the proper main repo mapping where appropriate

Use the main repo mapping (instead of ALWAYS_FALLBACK) to a) process .bzl load labels in the WORKSPACE file, and b) to process any labels passed to repo rules in either the WORKSPACE file or WORKSPACE-loaded macros.

This change only has a noticeable effect for when the workspace name (specified using the `workspace` directive in the WORKSPACE file) is used in labels as `@workspace_name//

***

PiperOrigin-RevId: 454899732
Change-Id: Ief9b90f981d3fdb551c53e56e9ffcfdfb9f61bd4
copybara-service bot pushed a commit that referenced this issue Jul 1, 2022
RepoContext = RepositoryName + RepositoryMapping
PackageContext = PackageIdentifier + RepositoryMapping
               = RepositoryName + PathFragment + RepositoryMapping
               = RepoContext + PathFragment

Making these one object stresses the fact that they often need to be used together for correctness.

Work towards #14852

PiperOrigin-RevId: 458437385
Change-Id: I68c0f61ebfcfbb2f5ff929d985bdfa0ac45a912a
copybara-service bot pushed a commit that referenced this issue Jul 6, 2022
We need to pass flag values through the repo mapping of the main repo, which means that at flag parsing time, we need access to the main repo mapping. To that end, we add a nullable untyped `conversionContext` parameter to the `Converter#convert` method, which is unused in this CL but will be used in a follow-up.

Note that we can't directly add a `RepositoryMapping` parameter because the c.g.devtools.common.options package is a transitive dependency of c.g.devtools.build.lib.cmdline (which RepositoryMapping lives in). So this `conversionContext` will unfortunately need to be an Object.

Reviewers: Please focus on reviewing changes in the c.g.devtools.common.options package. All the other changes in this CL are simply adding a `conversionContext` parameter to implementors of `Converter`, or passing this parameter to delegates, or superclasses.

Work towards #14852

PiperOrigin-RevId: 459278433
Change-Id: I98b3842305c34d2d0c33e7411c1024897fb0170a
copybara-service bot pushed a commit that referenced this issue Jul 11, 2022
…mapping

This CL adds an additional parsing round for all command-line options. The second round passes the main repo mapping as a conversion context, so any label-typed options can correctly go through repo mapping.

Similarly, when options are set in a Starlark transition, they'd also go through repo mapping. (except for Starlark-defined options, which will be fixed in a follow-up CL)

#14852

PiperOrigin-RevId: 460221378
Change-Id: I3aab3ee02cd5097743172edbe95d33d5e140300a
Wyverald added a commit that referenced this issue Jul 12, 2022
*** Reason for rollback ***

#15728 and #14852 have been fixed

*** Original change description ***

Automated rollback of commit 5de967b.

*** Reason for rollback ***

Temporarily rolling back until #14852 is fixed.

*** Original change description ***

Use the proper main repo mapping where appropriate

Use the main repo mapping (instead of ALWAYS_FALLBACK) to a) process .bzl load labels in the WORKSPACE file, and b) to process any labels passed to repo rules in either the WORKSPACE file or WORKSPACE-loaded macros.

This change...

***

PiperOrigin-RevId: 460471895
Change-Id: I60be60120aff003ebdb0d8d44ef82d412d81af92
copybara-service bot pushed a commit that referenced this issue Jul 13, 2022
*** Reason for rollback ***

#15728 and #14852 have been fixed

*** Original change description ***

Automated rollback of commit 5de967b.

*** Reason for rollback ***

Temporarily rolling back until #14852 is fixed.

*** Original change description ***

Use the proper main repo mapping where appropriate

Use the main repo mapping (instead of ALWAYS_FALLBACK) to a) process .bzl load labels in the WORKSPACE file, and b) to process any labels passed to repo rules in either the WORKSPACE file or WORKSPACE-loaded macros.

This change...

***

PiperOrigin-RevId: 460696618
Change-Id: I60be60120aff003ebdb0d8d44ef82d412d81af92
aranguyen pushed a commit to aranguyen/bazel that referenced this issue Jul 20, 2022
We need to pass flag values through the repo mapping of the main repo, which means that at flag parsing time, we need access to the main repo mapping. To that end, we add a nullable untyped `conversionContext` parameter to the `Converter#convert` method, which is unused in this CL but will be used in a follow-up.

Note that we can't directly add a `RepositoryMapping` parameter because the c.g.devtools.common.options package is a transitive dependency of c.g.devtools.build.lib.cmdline (which RepositoryMapping lives in). So this `conversionContext` will unfortunately need to be an Object.

Reviewers: Please focus on reviewing changes in the c.g.devtools.common.options package. All the other changes in this CL are simply adding a `conversionContext` parameter to implementors of `Converter`, or passing this parameter to delegates, or superclasses.

Work towards bazelbuild#14852

PiperOrigin-RevId: 459278433
Change-Id: I98b3842305c34d2d0c33e7411c1024897fb0170a
aranguyen pushed a commit to aranguyen/bazel that referenced this issue Jul 20, 2022
…mapping

This CL adds an additional parsing round for all command-line options. The second round passes the main repo mapping as a conversion context, so any label-typed options can correctly go through repo mapping.

Similarly, when options are set in a Starlark transition, they'd also go through repo mapping. (except for Starlark-defined options, which will be fixed in a follow-up CL)

bazelbuild#14852

PiperOrigin-RevId: 460221378
Change-Id: I3aab3ee02cd5097743172edbe95d33d5e140300a
aranguyen pushed a commit to aranguyen/bazel that referenced this issue Jul 20, 2022
*** Reason for rollback ***

bazelbuild#15728 and bazelbuild#14852 have been fixed

*** Original change description ***

Automated rollback of commit 5de967b.

*** Reason for rollback ***

Temporarily rolling back until bazelbuild#14852 is fixed.

*** Original change description ***

Use the proper main repo mapping where appropriate

Use the main repo mapping (instead of ALWAYS_FALLBACK) to a) process .bzl load labels in the WORKSPACE file, and b) to process any labels passed to repo rules in either the WORKSPACE file or WORKSPACE-loaded macros.

This change...

***

PiperOrigin-RevId: 460696618
Change-Id: I60be60120aff003ebdb0d8d44ef82d412d81af92
aranguyen pushed a commit to aranguyen/bazel that referenced this issue Jul 20, 2022
We need to pass flag values through the repo mapping of the main repo, which means that at flag parsing time, we need access to the main repo mapping. To that end, we add a nullable untyped `conversionContext` parameter to the `Converter#convert` method, which is unused in this CL but will be used in a follow-up.

Note that we can't directly add a `RepositoryMapping` parameter because the c.g.devtools.common.options package is a transitive dependency of c.g.devtools.build.lib.cmdline (which RepositoryMapping lives in). So this `conversionContext` will unfortunately need to be an Object.

Reviewers: Please focus on reviewing changes in the c.g.devtools.common.options package. All the other changes in this CL are simply adding a `conversionContext` parameter to implementors of `Converter`, or passing this parameter to delegates, or superclasses.

Work towards bazelbuild#14852

PiperOrigin-RevId: 459278433
Change-Id: I98b3842305c34d2d0c33e7411c1024897fb0170a
aranguyen pushed a commit to aranguyen/bazel that referenced this issue Jul 20, 2022
…mapping

This CL adds an additional parsing round for all command-line options. The second round passes the main repo mapping as a conversion context, so any label-typed options can correctly go through repo mapping.

Similarly, when options are set in a Starlark transition, they'd also go through repo mapping. (except for Starlark-defined options, which will be fixed in a follow-up CL)

bazelbuild#14852

PiperOrigin-RevId: 460221378
Change-Id: I3aab3ee02cd5097743172edbe95d33d5e140300a
aranguyen pushed a commit to aranguyen/bazel that referenced this issue Jul 20, 2022
*** Reason for rollback ***

bazelbuild#15728 and bazelbuild#14852 have been fixed

*** Original change description ***

Automated rollback of commit 5de967b.

*** Reason for rollback ***

Temporarily rolling back until bazelbuild#14852 is fixed.

*** Original change description ***

Use the proper main repo mapping where appropriate

Use the main repo mapping (instead of ALWAYS_FALLBACK) to a) process .bzl load labels in the WORKSPACE file, and b) to process any labels passed to repo rules in either the WORKSPACE file or WORKSPACE-loaded macros.

This change...

***

PiperOrigin-RevId: 460696618
Change-Id: I60be60120aff003ebdb0d8d44ef82d412d81af92
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 P2 We'll consider working on this in future. (Assignee optional) team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. type: bug
Projects
None yet
Development

No branches or pull requests

4 participants