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

Canonicalize use_extension label #17920

Closed
wants to merge 2 commits into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Mar 29, 2023

Canonicalize the label by adding the current module's repo_name if the
label doesn't specify a repository name. This is necessary as
ModuleExtensionUsages are grouped by the string value of this label, but
later mapped to their Label representation. If multiple strings map to
the same Label, this would result in a crash.

Also enforce that module() is called first (if at all).

@fmeum
Copy link
Collaborator Author

fmeum commented Mar 29, 2023

Stacked on #17909

@fmeum fmeum force-pushed the test-relative-extension-usage branch from 479d7e9 to db03c26 Compare March 29, 2023 21:04
@fmeum fmeum marked this pull request as ready for review March 29, 2023 21:04
@github-actions github-actions bot added awaiting-review PR is awaiting review from an assigned reviewer team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. labels Mar 29, 2023
@fmeum fmeum force-pushed the test-relative-extension-usage branch from db03c26 to 27e0dd8 Compare March 30, 2023 18:43
@fmeum
Copy link
Collaborator Author

fmeum commented Mar 30, 2023

@Wyverald I resolved the conflicts.

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.

very nice!

extensionBzlFile = "@" + ownName + rawExtensionBzlFile;
} else {
extensionBzlFile = rawExtensionBzlFile;
}
Copy link
Member

Choose a reason for hiding this comment

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

nit: could we put this part in a separate method? I think that'd help with readability since this part is very self-contained.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

Normalize the label by adding the current module's repo_name if the
label doesn't specify a repository name. This is necessary as
ModuleExtensionUsages are grouped by the string value of this label, but
later mapped to their Label representation. If multiple strings map to
the same Label, this would result in a crash.
@fmeum fmeum force-pushed the test-relative-extension-usage branch from 27e0dd8 to 0df7c14 Compare March 30, 2023 19:41
@sgowroji
Copy link
Member

Hi @Wyverald, Since I can see that this PR has been approved, please let me know whether I should proceed with importing it.Thanks!

@meteorcloudy meteorcloudy 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 Mar 31, 2023
Copy link
Member

@meteorcloudy meteorcloudy left a comment

Choose a reason for hiding this comment

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

Thanks!!

@sgowroji sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Mar 31, 2023
@fmeum fmeum deleted the test-relative-extension-usage branch April 1, 2023 09:43
Wyverald pushed a commit that referenced this pull request Apr 21, 2023
Canonicalize the label by adding the current module's repo_name if the
label doesn't specify a repository name. This is necessary as
ModuleExtensionUsages are grouped by the string value of this label, but
later mapped to their Label representation. If multiple strings map to
the same Label, this would result in a crash.

Also enforce that `module()` is called first (if at all).

Closes #17920.

PiperOrigin-RevId: 520890201
Change-Id: Ice8e2feb0da591e3ba953f4a85284766ba599ebf
fweikert pushed a commit to fweikert/bazel that referenced this pull request May 25, 2023
Canonicalize the label by adding the current module's repo_name if the
label doesn't specify a repository name. This is necessary as
ModuleExtensionUsages are grouped by the string value of this label, but
later mapped to their Label representation. If multiple strings map to
the same Label, this would result in a crash.

Also enforce that `module()` is called first (if at all).

Closes bazelbuild#17920.

PiperOrigin-RevId: 520890201
Change-Id: Ice8e2feb0da591e3ba953f4a85284766ba599ebf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants