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.

Sign up for GitHub

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

Jump to bottom

Toolchain exec/target_compatible_with using default_constraint_value #8274

Closed
m3rcuriel opened this issue May 9, 2019 · 6 comments
Closed

Toolchain exec/target_compatible_with using default_constraint_value #8274

m3rcuriel opened this issue May 9, 2019 · 6 comments
Assignees
@katre
Labels
team-Configurability Issues for Configurability team

Comments

@m3rcuriel
Copy link
Contributor

ATTENTION! Please read and follow:

Description of the problem / feature request:

I am not sure if this is a bug or a feature, but currently toolchains with exec_compatible_with/target_compatible_with seem to pull in a default constraint value. This means that you can't do something like the example below.

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

constraint_setting(
    name = "distro",
    default_constraint_value = ":ubuntu-bionic"
)

constraint_value(
   name = "ubuntu-bionic",
   constraint_setting = ":distro",
) 

constraint_value(
   name = "ubuntu-trusty",
   constraint_setting = ":distro",
) 

constraint_value(
   name = "ubuntu-disco",
   constraint_setting = ":distro",
)

Now the behavior I would expect would be that if I defined a toolchain with

exec_compatible_with = [
    "@//bazel_tools/platforms:whatever",
]

to work just fine. However, it uses the default_constraint_value for the toolchain instead.

This is presumably because

216:        toolchainConstraints.diff(platform.constraints());

is used to resolve the toolchains, and diff falls through on a default value on either other.constraints() or this.constraints(), and so the diff finds the default value.

Like I said, I'm not sure if this is a feature or a bug or if my intuition for constraint_settings is wrong. The issue from my point of view is that in order for toolchains to resolve correctly they must list out a value for every single constraint setting. This only makes sense if constraint settings are exclusively used to distinguish toolchains, in all toolchain_types.

What operating system are you running Bazel on?

Linux not4u 5.0.8-arch1-1-ARCH #1 SMP PREEMPT Wed Apr 17 14:56:15 UTC 2019 x86_64 GNU/Linux

What's the output of bazel info release?

AUR:

Build label: 0.25.0- (@non-git)
Build target: bazel-out/k8-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar
Build time: Tue May 7 11:54:47 2019 (1557230087)
Build timestamp: 1557230087
Build timestamp as int: 1557230087

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

This is an example output of --toolchain_resolution_debug from my project that isn't exactly as described but is pretty close:

INFO: ToolchainResolution: Looking for toolchain of type @io_bazel_rules_rust//rust:toolchain...
INFO: ToolchainResolution: Looking for toolchain of type @bazel_tools//tools/cpp:toolchain_type...
INFO: ToolchainResolution:   Considering toolchain //tools/rust:x86_64-linux-toolchain_impl...
INFO: ToolchainResolution:   Considering toolchain //tools/cpp:cc-compiler-bionic-x86_64...
INFO: ToolchainResolution:     Toolchain constraint //tools/platforms:distro has value //tools/platforms:ubuntu-bionic, which does not match value //tools/platforms:ubuntu-disco from the target platform //tools/platforms:disco-linux-x86_64
INFO: ToolchainResolution:     Toolchain constraint //tools/platforms:distro has value //tools/platforms:ubuntu-bionic, which does not match value //tools/platforms:ubuntu-disco from the target platform //tools/platforms:disco-linux-x86_64
INFO: ToolchainResolution:   Rejected toolchain //tools/rust:x86_64-linux-toolchain_impl, because of target platform mismatch
INFO: ToolchainResolution:   No toolchains found
INFO: ToolchainResolution:   Rejected toolchain //tools/cpp:cc-compiler-bionic-x86_64, because of target platform mismatch
INFO: ToolchainResolution:   Considering toolchain //tools/cpp:cc-compiler-disco-x86_64...
INFO: ToolchainResolution:     Toolchain constraint //tools/platforms:distro has value //tools/platforms:ubuntu-bionic, which does not match value //tools/platforms:ubuntu-disco from the target platform //tools/platforms:disco-linux-x86_64
INFO: ToolchainResolution:   Rejected toolchain //tools/cpp:cc-compiler-disco-x86_64, because of target platform mismatch
INFO: ToolchainResolution:   Considering toolchain @local_config_cc//:cc-compiler-armabi-v7a...
INFO: ToolchainResolution:     Toolchain constraint @bazel_tools//platforms:cpu has value @bazel_tools//platforms:arm, which does not match value @bazel_tools//platforms:x86_64 from the target platform //tools/platforms:disco-linux-x86_64
INFO: ToolchainResolution:     Toolchain constraint @bazel_tools//platforms:os has value @bazel_tools//platforms:android, which does not match value @bazel_tools//platforms:linux from the target platform //tools/platforms:disco-linux-x86_64
INFO: ToolchainResolution:     Toolchain constraint //tools/platforms:distro has value //tools/platforms:ubuntu-bionic, which does not match value //tools/platforms:ubuntu-disco from the target platform //tools/platforms:disco-linux-x86_64
INFO: ToolchainResolution:   Rejected toolchain @local_config_cc//:cc-compiler-armabi-v7a, because of target platform mismatch
INFO: ToolchainResolution:   Considering toolchain @local_config_cc//:cc-compiler-k8...
INFO: ToolchainResolution:     Toolchain constraint //tools/platforms:distro has value //tools/platforms:ubuntu-bionic, which does not match value //tools/platforms:ubuntu-disco from the target platform //tools/platforms:disco-linux-x86_64
INFO: ToolchainResolution:   Rejected toolchain @local_config_cc//:cc-compiler-k8, because of target platform mismatch
INFO: ToolchainResolution:   No toolchains found
ERROR: While resolving toolchains for target //:hello_world: no matching toolchains found for types @io_bazel_rules_rust//rust:toolchain
ERROR: Analysis of target '//:hello_world' failed; build aborted: no matching toolchains found for types @io_bazel_rules_rust//rust:toolchain
@irengrig irengrig added team-Configurability Issues for Configurability team untriaged labels May 9, 2019
@katre katre self-assigned this May 10, 2019
@katre katre removed the untriaged label May 10, 2019
@katre
Copy link
Member

katre commented May 10, 2019

I am a little confused by your example. when you say:

Now the behavior I would expect would be that if I defined a toolchain with

exec_compatible_with = [
    "@//bazel_tools/platforms:whatever",
]

What is @//bazel_tools/platforms/whatever?

Default constraint values are used whenever a platform or a toolchain defines a constraint value, and the other side of the comparison doesn't. So, in this case, if the platform includes any value for the :distro setting, if the toolchain doesn't define a specific setting, it takes the default, :ubuntu-bionic. However, if neither the platform nor the toolchain has a value for that constraint, it is skipped. (Formally, it could also be described that both sides take the default, and they match, so that is fine, but the actual implementation only looks at constraint settings that are in use in either side).

Do you have a minimal reproduction I can execute to see in more detail how the issue you are describing plays out?

@m3rcuriel
Copy link
Contributor Author

Based on your response I'm going to call this a feature not a bug.

I was saying take @//bazel_tools/platforms:whatever to be any toolchain constraint like @//bazel_tools/platforms:x86_64 as an example.

My point is that

Default constraint values are used whenever a platform or a toolchain defines a constraint value, and the other side of the comparison doesn't.

is more or less preventing me from using default values because it means that I must declare toolchains for any values of the default values that the toolchain "doesn't care about" because the platform does have those values defined. In this case, say that my Rust toolchain is distro-agnostic, but my C++ toolchain is not. This means I can write toolchain rules for the C++ toolchain as one would expect (say, clang-x86_64-bionic vs clang-x86_64-disco vs clang-x86_64-trusty), but I'm also forced to do rust-x86_64-bionic, etc) because I need to define a Rust toolchain for each possible value of //tools/platforms:distro or it will never resolve, even if the Rust toolchain doesn't care about that constraint.

Especially when it comes to target_compatible_with, go_toolchain is extremely platform agnostic, but I would have to list out every platform for those default values.

Hopefully that explains it better. Unfortunately I think it comes down to "the way you describe how this feature is supposed to work is weird to me," which would be a me issue more than a Bazel issue.

@katre
Copy link
Member

katre commented May 10, 2019

I think you have some good points and I want to carefully consider the usability aspect of defaults. I'm going to spend some time coming up with example cases and get back to this issue early next week.

@m3rcuriel
Copy link
Contributor Author

Thanks. Happy to help if you want me to produce anything.

@m3rcuriel m3rcuriel mentioned this issue Jul 4, 2019
Closed
@hlopko
Copy link
Member

hlopko commented Jul 5, 2019

We should merge these two threads (this and #8778) into one. @katre you get to pick one :)

@jmillikin jmillikin mentioned this issue Oct 25, 2019
Closed
bazel-io pushed a commit that referenced this issue Nov 5, 2019
@jmillikin
Avoid breaking toolchain resolution if platform has defaulted constra…
8404fb4 
…int.

### Summary
See #8778 and #8274 for context / motivation. In summary, the presence of a `constraint_setting` with `default_constraint_value` in a Platform's constraints will cause toolchain resolution to break for all toolchains that do not explicitly set that constraint value. This makes `default_constraint_value` dangerous because setting it can break other rulesets composed into the same top-level workspace.

cc @jayconrod @m3rcuriel @katre

### Test Plan
I updated the unit test `SingleToolchainResolutionFunction`, verified it failed, then altered the implementation to pass. I also built `//src:bazel` and ran it with a test workspace to verify the end result worked as expected.

Test workspace:
```
$ cat WORKSPACE
register_toolchains("//:my_toolchain")
$ cat BUILD
load(":rules.bzl", "my_toolchain_info", "my_rule")

my_rule(name = "hello")
toolchain_type(name = "my_toolchain_type")
my_toolchain_info(name = "my_toolchain_info")
toolchain(
    name = "my_toolchain",
    toolchain = "//:my_toolchain_info",
    toolchain_type = "//:my_toolchain_type",
)

constraint_setting(
    name = "setting",
    default_constraint_value = ":value_a",
)
constraint_value(
    name = "value_a",
    constraint_setting = ":setting",
)
constraint_value(
    name = "value_b",
    constraint_setting = ":setting",
)
platform(
    name = "platform_a",
    constraint_values = ["//:value_a"],
)
platform(
    name = "platform_b",
    constraint_values = ["//:value_b"],
)

$ cat rules.bzl
def _my_toolchain_info(ctx):
    return platform_common.ToolchainInfo()

my_toolchain_info = rule(_my_toolchain_info)

def _my_rule(ctx):
    pass

my_rule = rule(_my_rule, toolchains = ["//:my_toolchain_type"])
```

Before:
```
$ bazel build --toolchain_resolution_debug --host_platform=//:platform_b //:hello
[...]
INFO: ToolchainResolution: Looking for toolchain of type //:my_toolchain_type...
INFO: ToolchainResolution:   Considering toolchain //:my_toolchain_info...
INFO: ToolchainResolution:     Toolchain constraint //:setting has value //:value_a, which does not match value //:value_b from the target platform //:platform_b
INFO: ToolchainResolution:   Rejected toolchain //:my_toolchain_info, because of target platform mismatch
INFO: ToolchainResolution:   No toolchains found
ERROR: While resolving toolchains for target //:hello: no matching toolchains found for types //:my_toolchain_type
```

After:
```
$ bazel build --toolchain_resolution_debug --host_platform=//:platform_b //:hello
[...]
INFO: ToolchainResolution: Looking for toolchain of type //:my_toolchain_type...
INFO: ToolchainResolution:   Considering toolchain //:my_toolchain_info...
INFO: ToolchainResolution:   For toolchain type //:my_toolchain_type, possible execution platforms and toolchains: {//:platform_b -> //:my_toolchain_info}
INFO: ToolchainResolution: Selected execution platform //:platform_b, type //:my_toolchain_type -> toolchain //:my_toolchain_info
INFO: ToolchainResolution: Selected execution platform //:platform_b,
INFO: Analyzed target //:hello (9 packages loaded, 27 targets configured).
[...]
```

Closes #10102.

PiperOrigin-RevId: 278645171
@katre
Copy link
Member

katre commented Nov 5, 2019

I believe this is fixed by #10102. If so, please re-check and close the issue.

irengrig pushed a commit to irengrig/bazel that referenced this issue Nov 7, 2019
@jmillikin @irengrig
Avoid breaking toolchain resolution if platform has defaulted constra…
0af7c58 
…int.

### Summary
See bazelbuild#8778 and bazelbuild#8274 for context / motivation. In summary, the presence of a `constraint_setting` with `default_constraint_value` in a Platform's constraints will cause toolchain resolution to break for all toolchains that do not explicitly set that constraint value. This makes `default_constraint_value` dangerous because setting it can break other rulesets composed into the same top-level workspace.

cc @jayconrod @m3rcuriel @katre

### Test Plan
I updated the unit test `SingleToolchainResolutionFunction`, verified it failed, then altered the implementation to pass. I also built `//src:bazel` and ran it with a test workspace to verify the end result worked as expected.

Test workspace:
```
$ cat WORKSPACE
register_toolchains("//:my_toolchain")
$ cat BUILD
load(":rules.bzl", "my_toolchain_info", "my_rule")

my_rule(name = "hello")
toolchain_type(name = "my_toolchain_type")
my_toolchain_info(name = "my_toolchain_info")
toolchain(
    name = "my_toolchain",
    toolchain = "//:my_toolchain_info",
    toolchain_type = "//:my_toolchain_type",
)

constraint_setting(
    name = "setting",
    default_constraint_value = ":value_a",
)
constraint_value(
    name = "value_a",
    constraint_setting = ":setting",
)
constraint_value(
    name = "value_b",
    constraint_setting = ":setting",
)
platform(
    name = "platform_a",
    constraint_values = ["//:value_a"],
)
platform(
    name = "platform_b",
    constraint_values = ["//:value_b"],
)

$ cat rules.bzl
def _my_toolchain_info(ctx):
    return platform_common.ToolchainInfo()

my_toolchain_info = rule(_my_toolchain_info)

def _my_rule(ctx):
    pass

my_rule = rule(_my_rule, toolchains = ["//:my_toolchain_type"])
```

Before:
```
$ bazel build --toolchain_resolution_debug --host_platform=//:platform_b //:hello
[...]
INFO: ToolchainResolution: Looking for toolchain of type //:my_toolchain_type...
INFO: ToolchainResolution:   Considering toolchain //:my_toolchain_info...
INFO: ToolchainResolution:     Toolchain constraint //:setting has value //:value_a, which does not match value //:value_b from the target platform //:platform_b
INFO: ToolchainResolution:   Rejected toolchain //:my_toolchain_info, because of target platform mismatch
INFO: ToolchainResolution:   No toolchains found
ERROR: While resolving toolchains for target //:hello: no matching toolchains found for types //:my_toolchain_type
```

After:
```
$ bazel build --toolchain_resolution_debug --host_platform=//:platform_b //:hello
[...]
INFO: ToolchainResolution: Looking for toolchain of type //:my_toolchain_type...
INFO: ToolchainResolution:   Considering toolchain //:my_toolchain_info...
INFO: ToolchainResolution:   For toolchain type //:my_toolchain_type, possible execution platforms and toolchains: {//:platform_b -> //:my_toolchain_info}
INFO: ToolchainResolution: Selected execution platform //:platform_b, type //:my_toolchain_type -> toolchain //:my_toolchain_info
INFO: ToolchainResolution: Selected execution platform //:platform_b,
INFO: Analyzed target //:hello (9 packages loaded, 27 targets configured).
[...]
```

Closes bazelbuild#10102.

PiperOrigin-RevId: 278645171
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@katre katre

Labels
team-Configurability Issues for Configurability team
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@katre @hlopko @irengrig @m3rcuriel