cquery --output=starlark does not abide by semantics flags

c-parsons commented 1 year ago

Description of the bug:

I found this bug while fixing my project to support a recent flip of --incompatible_unambiguous_label_stringification, which is a significant change to Starlark semantics (briefly, stringifying any label in the main repository will return a string which begins the main repository prefix. That is, what was before //foo:bar is now @//foo:bar.)

My project is currently blocked and broken by this semantics change; we depend on cquery output for some tooling which is broken by this semantics change. A normal approach to defer a fix would be to add common --incompatible_unambiguous_label_stringification to the bazelrc, or to pass --incompatible_unambiguous_label_stringification to the cquery command. However, this seems to have no effect!

This is because cquery --output=starlark seems to always use the default Starlark semantics. That is, all starlark semantics flags are ignored in a cquery invocation.

This is the specific line in question: https://github.com/bazelbuild/bazel/blob/d69346575a7a2f791e61d181ca59aae6354236f9/src/main/java/com/google/devtools/build/lib/query2/cquery/StarlarkOutputFormatterCallback.java#L195

We should propagate Starlark Semantics to the cquery Starlark thread.

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

No response

Which operating system are you running Bazel on?

No response

What is the output of `bazel info release`?

No response

If `bazel info release` returns `development version` or `(@non-git)`, tell us how you built Bazel.

No response

What's the output of `git remote get-url origin; git rev-parse master; git rev-parse HEAD` ?

No response

Have you found anything relevant by searching the web?

No response

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

No response

brandjon commented 1 year ago

Marking release blocker since it sounds like an easy enough fix. Repeating what we discussed offline: You should be able to obtain the semantics using PrecomputedValue.STARLARK_SEMANTICS.get(env) on a skyframe Environment retrieved from the SkyframeExecutor, and store that inside the object at construction for later use in the other method. Would you mind implementing since you have the repro handy? I don't think it needs a test...

c-parsons commented 1 year ago

Sure thing. Will follow up by next week.

meteorcloudy commented 1 year ago

I assume you want this to be fixed before 6.0 cut? Therefore, adding the 6.0.0 release blockers milestone.

c-parsons commented 1 year ago

I think it should be, yes :) Thanks, Yun!

lberki commented 1 year ago

@brandjon what's the impact of this issue? All things being equal, I'd much rather this doesn't take energy away from e.g. .bzl visibility and I don't immediately see why this is urgent enough to warrant the status of being a release blocker.

lberki commented 1 year ago

Bumping this down to P2 to indicate our collective desire to get this into Bazel 6.0, but also that we won't postpone the release cut for this, should this and other P2s be the only things that remain.

c-parsons commented 1 year ago

That's fine. Having this issue fixed in 6.0 is not something that matters to my project, for a couple of reasons:

--incompatible_unambiguous_label_stringification no longer breaks us in either the true or false case (the root issue where I discovered this underlying bug).; we've made our integration resilient to the value of this flag.
Our project cuts a Bazel at HEAD frequently. We won't be using the Bazel 6.0 Release long term.

brandjon commented 1 year ago

(Taking myself off assignees as this is a configurability issue, but happy to consult.)

bazelbuild / bazel