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

Support contraint_values in select()s for alias rules #13101

Closed
wants to merge 1 commit into from

Conversation

staalb
Copy link

@staalb staalb commented Feb 24, 2021

Fixes #13047

Tested by running bazel test //src/test/java/com/google/devtools/build/lib/...

@staalb staalb requested a review from lberki as a code owner February 24, 2021 23:37
@google-cla google-cla bot added the cla: yes label Feb 24, 2021
@lberki lberki requested review from katre and removed request for lberki February 25, 2021 09:31
@staalb
Copy link
Author

staalb commented Feb 25, 2021

This is getting a bit complicated for me. It's hard to determine which tests to run on my system (couldn't find any docs on the subject other than CODEBASE.md). I cannot just run bazel test //... on my system, e.g. because I don't have an Android SDK installed.

There are a lot of build and test failures. Didn't have the time yet to go through all of them, but I found these so far:

  • Test //src/test/java/com/google/devtools/build/lib/analysis:AnalysisTests fails on BuidKite, while it runs just fine on my system. For a system integration test this wouldn't suprise me too much. However, this is a "unit test" which only runs on the analysis phase.
  • Test //src/test/shell/bazel:platform_mapping_test fails too. If I undo the change it passes again. I'll try to investigate this, but I'm afraid I have far too little knowledge of the Bazel codebase... Anyway, I give it a try.

@katre
Copy link
Member

katre commented Feb 25, 2021

Okay, took a look at your test failures. I'm actually surprised that this passes anywhere. The underlying problem is the reason why we first exempted alias from toolchain resolution: there are aliases used in defining core constraints.

Take a look at the BUILD file for @bazel_tools//platforms. You'll see that these constraints are all actually aliases for canonical values from the @platforms repo. If alias targets required toolchain resolution, you'd see the following:

  1. Bazel attempts to analyze a target, say //java/package:foo
  2. Bazel needs to perform toolchain resolution for that target, so it begins analyzing the target platform. This then includes also analyzing the constraints for that platform.
    1. Bazel analyzes the platform and constraints in the same configutation as //java/package:foo. There's not actually a good reason for this, but we have to analyze them in some configuration, and this one is handy. We've discussed the concept of "null-configured targets" before, but didn't get very far with this because there are a lot of assumptions in Bazel about targets always having a configuration.
  3. When Bazel analyzes one of the constraints that happens to be an alias, with your change it now needs to perform toolchain resolution
  4. We now have a dependency cycle, because in order to finish toolchain resolution for the original target we've re-invoked toolchain resolution with essentially the same arguments. In your tests this shows up as an unreported dependency cycle, because there's no special handling for this case.

I'm afraid that your change isn't going to be submittable as-is without a lot of underlying changes to how Bazel handles toolchain resolution. I'm going to copy this comment back to the original issue so we can continue discussing potential solutions there.

@jin jin added the team-Configurability platforms, toolchains, cquery, select(), config transitions label Mar 1, 2021
@staalb
Copy link
Author

staalb commented Mar 24, 2021

Thanks for looking into the test results and your explanation. From the discusison in the bug report I understand a complete different approach is necessary for this problem, so I'll close this PR.

@staalb staalb closed this Mar 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes team-Configurability platforms, toolchains, cquery, select(), config transitions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Not possible to select() on constraint_value in alias rules
3 participants