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

Fix query command suggestion in error message with repo mappings #16482

Closed
wants to merge 1 commit into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Oct 16, 2022

The package label in the query command suggested when a specified target isn't found in a package looked like @rules_cc~1.0.0//pkg, which doesn't work as the repo name is canonical, but the label isn't.

This is fixed by reusing the logic from Label#getUnambiguousCanonicalForm.

@fmeum fmeum force-pushed the fix-query-hints branch 2 times, most recently from f387d43 to 95bda29 Compare October 16, 2022 09:45
@fmeum fmeum marked this pull request as ready for review October 16, 2022 10:25
@fmeum
Copy link
Collaborator Author

fmeum commented Oct 16, 2022

@meteorcloudy @Wyverald Could you review this?

I noticed that the output of query consists of non-canonical label literals with canonical repo names. Those can't really be used anywhere with Bzlmod enabled. Should query emit canonical label literals instead, at least with Bzlmod?

Edit: I looked into how this could be improved. It doesn't seem that difficult to get query output to use this output format instead: //pkg:file for targets in the main repo, @my_dep//pkg:file for targets in a repo that the main repo has a local name for and @@dep~1.0.0//pkg:file when the repo isn't visible from the main repo. What do you think, is that the right direction?

@sgowroji sgowroji added team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. area-Bzlmod Bzlmod-specific PRs, issues, and feature requests awaiting-review PR is awaiting review from an assigned reviewer labels Oct 17, 2022
@Wyverald Wyverald added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Oct 18, 2022
@Wyverald
Copy link
Member

I noticed that the output of query consists of non-canonical label literals with canonical repo names. Those can't really be used anywhere with Bzlmod enabled. Should query emit canonical label literals instead, at least with Bzlmod?

I think to begin with, outputting @@-style labels when Bzlmod is enabled should suffice; but what you described later is even better, if it's not too much trouble to implement.

@meteorcloudy meteorcloudy added awaiting-user-response Awaiting a response from the author and removed awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally labels Oct 18, 2022
Copy link
Member

@Wyverald Wyverald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, Yun just pointed out to me that this would break Google, where we don't use external repos at all and @-prefixed labels are forbidden.

Could you instead only turn on this behavior when Bzlmod is enabled?

@fmeum
Copy link
Collaborator Author

fmeum commented Oct 18, 2022

Instead of depending on the Bzlmod bit explicitly, I backported the "one label stringification function to rule them all" I came up with for #16483, which decanonicalizes query output as far as possible, to the current PR. I moved the tests over since the current PR doesn't exercise all its functionality yet.

It is slightly weird that the new function takes in a repo mapping of either the main or any other repository, but I found that to be the simplest pattern to gradually improve the existing cases of label formatting. Checking for strict visibility specifically seemed cleaner than testing for Bzlmod specifically, even though the should be (mostly?) equivalent. Looking forward to hearing your thoughts on this!

@fmeum fmeum force-pushed the fix-query-hints branch 2 times, most recently from d93a9b2 to a2342e8 Compare October 19, 2022 17:26
The package label in the query command suggested when a specified target
isn't found in a package looked like `@rules_cc~1.0.0//pkg`, which
doesn't work with Bzlmod as the repo name is canonical, but the label
isn't.

This is fixed by introducing a new function `getDisplayForm` on
`PackageIdentifier` that prints the most readable label representation
of a package from the context of the main repository given a repository
mapping (ideally that of the main repository).

This function will be used in a follow-up PR to decanonicalize labels in
{a,c,}query output when possible.
@Wyverald Wyverald added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-user-response Awaiting a response from the author labels Oct 19, 2022
@fmeum fmeum deleted the fix-query-hints branch October 24, 2022 13:16
@Wyverald Wyverald removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Oct 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Bzlmod Bzlmod-specific PRs, issues, and feature requests team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants