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

Breaking change in Bazel CQuery due to Starlarkification of providers #20473

Open
guw opened this issue Dec 8, 2023 · 15 comments
Open

Breaking change in Bazel CQuery due to Starlarkification of providers #20473

guw opened this issue Dec 8, 2023 · 15 comments
Labels
P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) team-Rules-Java Issues for Java rules type: support / not a bug (process)

Comments

@guw
Copy link
Contributor

guw commented Dec 8, 2023

Description of the bug:

The following cquery no longer works with Bazel 7:

bazel cquery "@bazel_tools//tools/jdk:current_java_toolchain" --output starlark --starlark:expr 'providers(target)["JavaToolchainInfo"].source_version'

It looks like that providers(target)["JavaToolchainInfo"] needs to be replaced with providers(target)['@@_builtins//:common/java/java_toolchain.bzl%JavaToolchainInfo'].source_version but it feels odd to require this verbosity now. Especially the label syntax looks subjective to further changes. It should be possible to select a provider without relying on the location.

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

echo "7.0.0.rc5" > .bazelversion

run the query

Have you found anything relevant by searching the web?

https://bazelbuild.slack.com/archives/CDCE4DFAP/p1702038149142419?thread_ts=1701990164.906629&cid=CDCE4DFAP

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

This needs to be documented. Ideally all those queries would continue to work, though.

@meisterT
Copy link
Member

meisterT commented Dec 8, 2023

cc @gregestren

@meisterT
Copy link
Member

meisterT commented Dec 8, 2023

@guw did you already bisect which change caused this change in behavior?

@meteorcloudy
Copy link
Member

Maybe this is just some breaking change after we starlarkifying JavaToolchainInfo? @comius

@meteorcloudy
Copy link
Member

8eecb29 /cc @hvadehra

@hvadehra
Copy link
Member

hvadehra commented Dec 8, 2023

Yes, this will be a side-effect of moving native providers to Starlark.

The providers() function uses the Starlark provider's key.

Not sure anything else would make sense for identifying a provider written in Starlark. So we should just add this to the release notes as a breaking change.

@meteorcloudy
Copy link
Member

Thanks for confirming!

So we should just add this to the release notes as a breaking change.

Please do!

@guw
Copy link
Contributor Author

guw commented Dec 8, 2023

Not sure anything else would make sense for identifying a provider written in Starlark. So we should just add this to the release notes as a breaking change.

I think that would be super helpful (although inconvenient). Would it be possible to provide a different function/way?

@comius
Copy link
Contributor

comius commented Dec 8, 2023

I’m not happy with this, but this is WAI and will change at least one more time when the rules are moved out of builtins.

Making it same as before would require adding hacks to the implementation.

Any bzl file that needs a provider, has to import it via a load. This is also going to change from a global symbol for even JavaInfo to a load from rules_java.

Starlark query expressions are the only location where a provider can’t be loaded and you refer to it by a string. For correctness, also bzl file needs to be in the string (otherwise the map might not have unique keys).

Possible workaround in user space is to either use both strings that represent the provider. Or maybe even filter the map for keys that end with JavaTolchainInfo

@gregestren
Copy link
Contributor

@gu2 can you think of any clean user experience that wouldn't have the ambiguity of filtering the map? (i.e. risk of resolving to the wrong JavaToolchainInfo if not fully qualified)?

@guw
Copy link
Contributor Author

guw commented Dec 9, 2023

It depends on how real ambiguity is @gregestren. It would love to have a find_provider(target, "JavaToolchainInfo") helper that fails if there is none or multiple. Still trying to wrap my head around how to do this in a single line.

Perhaps there is a better way. The full query I am doing is:

bazel cquery --output starlark --starlark:expr "providers(target)['JavaToolchainInfo'].source_version + '::' + providers(target)['JavaToolchainInfo'].target_version + '::' + providers(target)['JavaToolchainInfo'].java_runtime.java_home" @bazel_tools//tools/jdk:current_java_toolchain

I would have to do the filtering three times in a single line. I don't think --starlark:expr allows assignments. I haven't it figured out yet.

All I want is identify JAVA_HOME and source and target version so I can setup the IDE properly. Perhaps there is a better way to query this?

@hvadehra
Copy link
Member

hvadehra commented Dec 9, 2023

I would have to do the filtering three times in a single line. I don't think --starlark:expr allows assignments. I haven't it figured out yet.

Is it necessary for the starlark code to be specified on the command line? If not, you could use --starlark:file instead. There are examples in the docs.

@guw
Copy link
Contributor Author

guw commented Dec 9, 2023

Oh, that looks doable.

BTW, that example doc needs fixing as well:

providers(target) returns a map whose keys are names of providers (for example, "DefaultInfo").

It should mention the specifics around keys instability from Bazel version to version.

guw added a commit to salesforce/bazel-eclipse that referenced this issue Dec 9, 2023
This adapts to breaking changes in Bazel 7 while maintaining backwards
compatibility to Bazel 6.x.

See bazelbuild/bazel#20473 for details.
@guw
Copy link
Contributor Author

guw commented Dec 9, 2023

Thanks @hvadehra. Using --starlark:file works much better for this.

@tpasternak
Copy link
Contributor

so the issue remains open, does it mean it will be fxied at some point, or we should treat @@_builtins//:common/java/java_info.bzl%JavaInfo provider name as the official way to get it in queries?

@hvadehra
Copy link
Member

hvadehra commented Jan 8, 2024

we should treat @@_builtins//:common/java/java_info.bzl%JavaInfo provider name as the official way to get it in queries

Yes, for now that will the key. As noted in #20473 (comment) it will change (at least) once more when the java rules move out of @builtins and into @rules_java (likely in Bazel 8). Given this issue, we should keep the provider location(s) unchanged after that point in time.

@hvadehra hvadehra added P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) type: support / not a bug (process) and removed type: bug untriaged labels Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) team-Rules-Java Issues for Java rules type: support / not a bug (process)
Projects
None yet
Development

No branches or pull requests

10 participants