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

cquery --show_config_fragments: support options read by transitions #11258

Closed
gregestren opened this issue Apr 29, 2020 · 1 comment
Closed

cquery --show_config_fragments: support options read by transitions #11258

gregestren opened this issue Apr 29, 2020 · 1 comment
Assignees
@gregestren
Labels
P1 I'll work on this now. (Assignee required) team-Configurability platforms, toolchains, cquery, select(), config transitions type: feature request

Comments

@gregestren
Copy link
Contributor

Support work for #10613.

Configuration transitions can themselves read flags to figure out what to do.

Example:

bazel/src/main/java/com/google/devtools/build/lib/rules/python/PythonVersionTransition.java

Line 76 in f0a40ac

PythonOptions opts = options.get(PythonOptions.class);

This means if you build //foo:a with --someflag=1 and repeat the build with --someflag=0, //foo:a may produce different output even though the rule doesn't directly read --someflag.

So we need to factor in what transitions attached to the rule read too.

Plan:

  1. Add a new RestrictedBuildOptions that wraps BuildOptions but only allows BuildOptions.get() for pre-registered fragment classes.
  2. Have every ConfigurationTransition declare which fragment classes it reads
  3. Change ConfigurationTransition.apply(BuildOptions) to ConfigurationTransition.apply(RestrictedBuildOptions).
  4. Have cquery --show_config_fragments include these fragments.
  5. Implement this new interface accurately for Starlark transitions.
@gregestren gregestren added type: feature request P1 I'll work on this now. (Assignee required) team-Configurability platforms, toolchains, cquery, select(), config transitions labels Apr 29, 2020
@gregestren gregestren self-assigned this Apr 29, 2020
@gregestren
Copy link
Contributor Author

P1 review: this is still being actively worked on. I have a PR for the first two(ish) steps in transit.

@gregestren gregestren mentioned this issue May 11, 2020
Closed
bazel-io pushed a commit that referenced this issue May 19, 2020
@gregestren @copybara-github
Introduce configuration transition fragment declarations.
ecb61ee 
See #11258 for context.

This essentially converts ConfigurationTransition.apply(BuildOptions...)
to ConfigurationTransition.apply(RestrictedBuildOptions...). The new
BuildOptionsView wraps BuildOptions while only allowing access
to options fragments the transitions declare.

Because there are a *lot* of uses of PatchTransition and SplitTransition,
this change only adds a parallel interface to PatchTransition.patch and
SplitTransition.split. Implementers continue to work as-is with the
BuildOptions variant, while we can incrementally switch them over to
the RestrictedBuildOptions variant at whatever pace we desire.

This helps decouple risk of failing CI if/when we accidentally forget
to migrate a few implementers or get their set of declared fragments wrong.

This implements step 1 of #11258.

PiperOrigin-RevId: 312311815
bazel-io pushed a commit that referenced this issue Jun 9, 2020
@gregestren @copybara-github
Move some native transitions to BuildOptionsView.
dc08d65 
In service of #11258.

PiperOrigin-RevId: 315577744
bazel-io pushed a commit that referenced this issue Jun 11, 2020
@gregestren @copybara-github
Convert remaining native uses of patch() / split().
a048f0f 
In service of #11258.

PiperOrigin-RevId: 315986864
bazel-io pushed a commit that referenced this issue Jun 19, 2020
@gregestren @copybara-github
Report fragments Starlark transitions use.
06b76c9 
Also add some straggler native transitions.

With all transitions converted, also remove legacy
PatchTransition.patch(BuildOptions) and
SplitTransition.split(BuildOptions).

In service of #11258.

PiperOrigin-RevId: 317383960
bazel-io pushed a commit that referenced this issue Jul 1, 2020
@gregestren @copybara-github
cquery --show_config_fragments: report fragments used by transitions
24e09f7 
Also:

 1 Move collection logic into its own utility class to declutter a
   hard-enough-to-understand core class
 2 Change the signature on Starlark transition requirements from fragment classes
   to the user-friendly strings they ultimately get transformed to. We do this
   early because transitions report options, not fragments. Trying to reduce them
   to fragments involves a way more complicated API with no real benefit
   (particularly for Starlark flags and CoreOptions, which have no fragment).

   This change has some quirks, which is why I changed some existing test
   signatures. But it's far preferable IMO to overly complicating Blaze's core APIs

One of 2's particular quirks is that the final "required fragments" string isn't consistent. For example, if a rule definition requires the C++ fragment it returns "CppConfiguration", whereas if a select() or transition requires a C++ option it returns "CppOptions". This is arguably confusing. But the alternative of forcing everything into a proper Fragment (CppConfiguration) requires passing a FragmentOptions -> Fragment map to all code that computes FragmentOptions, which really complicates Blaze's APIs.

Furthermore, "blaze config" outputs a mapping of fragments to FragmentOptions, which means consumers have all the info they need to connect and deduplicate.

The result is a minor inconvenience that's not too invasive to Blaze and doesn't compromise ultimate correctness.

In service of #11258.

PiperOrigin-RevId: 319306418
luca-digrazia pushed a commit to luca-digrazia/DatasetCommitsDiffSearch that referenced this issue Sep 4, 2022
Report fragments Starlark transitions use.
e9df268 
    Also add some straggler native transitions.

    With all transitions converted, also remove legacy
    PatchTransition.patch(BuildOptions) and
    SplitTransition.split(BuildOptions).

    In service of bazelbuild/bazel#11258.

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

@gregestren gregestren

Labels
P1 I'll work on this now. (Assignee required) team-Configurability platforms, toolchains, cquery, select(), config transitions type: feature request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@gregestren