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

Identify isolated extensions by exported name #18840

Closed
wants to merge 1 commit into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Jul 4, 2023

Instead of making a running counter and the dev_dependency bit the
identifier of an isolated module extension usage within a module file,
use the exported name of the usage. This still results in a stable name
even with --ignore_dev_dependency flips, but makes the canonical
repository name more recognizable and allows for less verbose buildozer
fixup commands.

@fmeum
Copy link
Collaborator Author

fmeum commented Jul 4, 2023

Stacked on #18829

@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 Jul 4, 2023
@fmeum fmeum force-pushed the isolate-by-exported-name branch from 734471e to 2ccd108 Compare July 4, 2023 22:57
Instead of making a running counter and the `dev_dependency` bit the
identifier of an isolated module extension usage within a module file,
use the exported name of the usage. This still results in a stable name
even with `--ignore_dev_dependency` flips, but makes the canonical
repository name more recognizable and allows for less verbose buildozer
fixup commands.
@fmeum
Copy link
Collaborator Author

fmeum commented Jul 12, 2023

@Wyverald I rebased onto master.

@fmeum
Copy link
Collaborator Author

fmeum commented Jul 12, 2023

@bazel-io flag

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Jul 12, 2023
@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 Jul 12, 2023
@iancha1992
Copy link
Member

@bazel-io fork 6.3.0

@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Jul 12, 2023
@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Jul 12, 2023
@iancha1992
Copy link
Member

iancha1992 commented Jul 12, 2023

@fmeum @Wyverald looks like there's a conflict when attemting a cherry-pick to release-6.3.0. from the "src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunction.java"

"bestName" variable should not be:
String bestName = nonEmptyRepoPart + "~" + id.getExtensionName();

Instead, should be somewhat looks like below before the cherry-pick:

String bestName = id.getIsolationKey() .map( namespace -> String.format( "%s~_%s~%s~%s~%s", nonEmptyRepoPart, id.getExtensionName(), namespace.getModule().getName(), namespace.getModule().getVersion(), namespace.getUsageExportedName())) .orElse(nonEmptyRepoPart + "~" + id.getExtensionName());

Can you please take a look and let us know which commit to push in order to resolve this? Thanks!

cc: @bazelbuild/triage

@Wyverald
Copy link
Member

It's safe to just use the longer snippet you pasted.

@fmeum fmeum deleted the isolate-by-exported-name branch July 12, 2023 21:40
iancha1992 pushed a commit to iancha1992/bazel that referenced this pull request Jul 12, 2023
Instead of making a running counter and the `dev_dependency` bit the
identifier of an isolated module extension usage within a module file,
use the exported name of the usage. This still results in a stable name
even with `--ignore_dev_dependency` flips, but makes the canonical
repository name more recognizable and allows for less verbose buildozer
fixup commands.

Closes bazelbuild#18840.

PiperOrigin-RevId: 547587673
Change-Id: I2137ed83f1600c00f73539c8a3f002268e4c0476
iancha1992 pushed a commit to iancha1992/bazel that referenced this pull request Jul 12, 2023
Instead of making a running counter and the `dev_dependency` bit the
identifier of an isolated module extension usage within a module file,
use the exported name of the usage. This still results in a stable name
even with `--ignore_dev_dependency` flips, but makes the canonical
repository name more recognizable and allows for less verbose buildozer
fixup commands.

Closes bazelbuild#18840.

PiperOrigin-RevId: 547587673
Change-Id: I2137ed83f1600c00f73539c8a3f002268e4c0476
@keertk
Copy link
Member

keertk commented Jul 13, 2023

@fmeum @Wyverald we're not able to get this into rc1 (to go out shortly) unfortunately - there are still some failures in #18923.

Please let us know if this cherry-pick is required and we can create another release candidate in the coming days. Thanks!

Edit: pushed RC1 to 7/13, so we still have a day to get this in if needed.

@fmeum
Copy link
Collaborator Author

fmeum commented Jul 13, 2023

See #18923 (comment), a previous cherry-pick ended up partially reverting an earlier one.

iancha1992 added a commit that referenced this pull request Jul 13, 2023
Instead of making a running counter and the `dev_dependency` bit the
identifier of an isolated module extension usage within a module file,
use the exported name of the usage. This still results in a stable name
even with `--ignore_dev_dependency` flips, but makes the canonical
repository name more recognizable and allows for less verbose buildozer
fixup commands.

Closes #18840.

PiperOrigin-RevId: 547587673
Change-Id: I2137ed83f1600c00f73539c8a3f002268e4c0476

Co-authored-by: Fabian Meumertzheim <fabian@meumertzhe.im>
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.

5 participants