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

Accept config_settings in target_compatible_with #12614

Closed
chsigg opened this issue Dec 3, 2020 · 7 comments
Closed

Accept config_settings in target_compatible_with #12614

chsigg opened this issue Dec 3, 2020 · 7 comments
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Configurability platforms, toolchains, cquery, select(), config transitions type: feature request

Comments

@chsigg
Copy link

chsigg commented Dec 3, 2020

Description of the feature request:

bazel 4.0 adds a target_compatible_with common attribute that accepts a list constraint_values. It would be great if it could also accept config_settings.

What underlying problem are you trying to solve with this feature?

I would like to mark a target to be compatible only if :my_config_setting is true, so that the target itself and all inverse dependencies don't build otherwise.

I can currently work around this by translating a config_setting into a constraint_value by select()ing the constraint_setting's default_constraint_value. But apparently constraint_settings are supposed to be configuration independent, so that workaround is bad style:

config_setting(
  name = "my_config",
  ...
)
constraint_setting(
  name = "my_constraint",
  default_constraint_value = select({
    ":my_config": ":my_value",
    "//conditions_default": None,
  }),
)
constraint_value(
  name = "my_value",
  constraint_setting = ":my_constraint",
)
cc_binary(
  ...
  target_compatible_with = [
    # feature request: allow ":my_config" here instead
    ":my_value",
  ],
)
@jin jin added team-Configurability platforms, toolchains, cquery, select(), config transitions type: feature request untriaged labels Dec 3, 2020
@gregestren gregestren added P3 We're not considering working on this, but happy to review a PR. (No assignee) and removed untriaged labels Dec 4, 2020
@gregestren
Copy link
Contributor

Interesting. What conditions are you interested in?

CC @AustinSchuh @philsc

@gregestren
Copy link
Contributor

We've also had some interesting discussion on compatibility based on toolchain properties (like compiler type) from the advent of this feature. So maybe there's conceptual overlap?

@chsigg
Copy link
Author

chsigg commented Dec 4, 2020

What conditions are you interested in?

I hope I understand your question correctly, that you are referring to the attributes of config_setting. I would probably only need config_settings that use the flag_values attribute.

@AustinSchuh
Copy link
Contributor

@gregestren, This is in our original proposal, but was deferred until a second phase so we could get something working first. It was how I've been thinking about implementing toolchain property based skipping. So yea, sounds great!

@philsc
Copy link
Contributor

philsc commented Dec 5, 2020

Just so I understand properly. Is this functionally equivalent to creating a helper like this?

def config_compatible(config_setting_target):
    return select({
        config_setting_target: [],
        "//conditions:default": ["@platforms//:incompatible"],
    })

and then using it like so:

cc_binary(
  ...
  target_compatible_with = config_compatible(":my_value"),
)

Pretty sure you can do the above today.

Again, so I'm clear exactly what we're talking about, you'd like to get rid of the need for a helper function like config_compatible() above?

@chsigg
Copy link
Author

chsigg commented Dec 6, 2020

Ha! Thanks a lot for thinking of this Philipp.

Yes, selecting on the target_compatible_with would be a perfectly fine alternative. I don't mind the helper function.

In fact, that's what I used before I realized that you can (currently, and I hear it might go away) select on the default_constraint_value. But by the time I wrote this issue I had already forgotten :-(

I'm happy with Philipp's approach and as far as I'm concerned you can close this issue.

Thanks for your help!

@gregestren
Copy link
Contributor

Closing as acknowledged.

To be clear, my preference is to try to limit built-in Bazel support to constraint_value and the mentioned to-be-implemented toolchain properties.

I like that a reasonable user-space approach was identified here, so you can get what you need without stretching Bazel's core API complexity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Configurability platforms, toolchains, cquery, select(), config transitions type: feature request
Projects
None yet
Development

No branches or pull requests

5 participants