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.

Sign up for GitHub

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

Jump to bottom

Allow module extension usages to be isolated #18529

Closed
wants to merge 5 commits into from
Closed

Allow module extension usages to be isolated #18529

wants to merge 5 commits into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented May 30, 2023 •
edited
Loading

If isolate = True is specified on use_extension, that particular usage will be isolated from all other usages, both in the same and other modules.

Module extensions can check whether they are isolated (e.g. in case they can only be used in this way) via module_ctx.is_isolated.

@fmeum fmeum force-pushed the isolated branch 4 times, most recently from 4d34d12 to c816feb Compare May 30, 2023 14:21
aherrmann
aherrmann reviewed Jun 8, 2023
View reviewed changes
Copy link
Contributor

@aherrmann aherrmann left a comment

Choose a reason for hiding this comment

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

I like this idea. I think this would help with the distinction between globally unified and local packages in rules_nixpkgs, see here. The globally unified ones would be provided by a regular extension, and the local ones by an isolated extension.

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java Show resolved Hide resolved
src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java Outdated Show resolved Hide resolved
@fmeum
Copy link
Collaborator Author

fmeum commented Jun 9, 2023

I added tests as well as module_ctx.extension_metadata support. I also changed the naming scheme, see the comment in BazelDepGraphFunction.

@Wyverald bazelbuild/buildtools#1171 is the corresponding change to buildozer.

@fmeum fmeum marked this pull request as ready for review June 9, 2023 19:32
@fmeum fmeum requested a review from meteorcloudy as a code owner June 9, 2023 19:32
@fmeum fmeum requested a review from Wyverald June 9, 2023 19:33
@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 Jun 9, 2023
@fmeum
Copy link
Collaborator Author

fmeum commented Jun 10, 2023 •
edited
Loading

Gson doesn't seem to handle Optional<T> out of the box. @SalmaSamy Do you happen to know what the best approach is to manually add support for it?

Edit: I found a way, but it's rather verbose.

@fmeum fmeum force-pushed the isolated branch 4 times, most recently from c83e00e to 23291be Compare June 11, 2023 19:57
@SalmaSamy
Copy link
Contributor

SalmaSamy commented Jun 12, 2023 •
edited
Loading

Do you happen to know what the best approach is to manually add support for it?

Unfortunately not. Probably writing an adapter for it, though you will still need to handle the object type somehow Ex: via TypeToken or something.

@fmeum
Copy link
Collaborator Author

fmeum commented Jun 12, 2023

I ended up writing my own TypeAdapterFactory: 23291be

Wyverald
Wyverald approved these changes Jun 14, 2023
View reviewed changes
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.

nice!

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java Show resolved Hide resolved
src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java Outdated Show resolved Hide resolved
src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java Outdated Show resolved Hide resolved
@fmeum
Allow module extension usages to be isolated
b3afe0c 
If `isolated = True` is specified on `use_extension`, that particular
usage will be isolated from all other usages, both in the same and other
modules.

Module extensions can check whether they are isolated (e.g. in case they
can only be used in this way) via `module_ctx.is_isolated`.
@fmeum
Add test, address feedback, use new naming scheme
234c824
@fmeum
Add TypeAdapterFactory for Optional
51e3df3
@fmeum
Fix incorrect proxy builder reuse
e4ce1be
@fmeum
Address review comments
d2d23ff
@fmeum fmeum requested a review from Wyverald June 15, 2023 14:41
@fmeum
Copy link
Collaborator Author

fmeum commented Jun 15, 2023

Just in case you missed it: I changed the name of the parameter from isolated to isolate as I felt that matches better with use_extension. Let me know if you don't like this "bikeshe" :-)

@fmeum fmeum mentioned this pull request Jun 15, 2023
Merged
@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 Jun 15, 2023
@fmeum fmeum deleted the isolated branch June 20, 2023 08:48
@sgowroji sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Jun 20, 2023
@fmeum
Copy link
Collaborator Author

fmeum commented Jun 20, 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 Jun 20, 2023
@meteorcloudy
Copy link
Member

@bazel-io fork 6.3.0

@bazel-io bazel-io mentioned this pull request Jun 20, 2023
Closed
@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 Jun 20, 2023
iancha1992 pushed a commit to iancha1992/bazel that referenced this pull request Jun 20, 2023
@fmeum @iancha1992
Allow module extension usages to be isolated
7e2e245 
If `isolate = True` is specified on `use_extension`, that particular usage will be isolated from all other usages, both in the same and other modules.

Module extensions can check whether they are isolated (e.g. in case they can only be used in this way) via `module_ctx.is_isolated`.

Closes bazelbuild#18529.

PiperOrigin-RevId: 541823020
Change-Id: I68a7b49914bbc1fd50df2fda7a0af1e47421bb92
@iancha1992 iancha1992 mentioned this pull request Jun 20, 2023
Merged
iancha1992 added a commit that referenced this pull request Jun 21, 2023
@iancha1992 @fmeum
Allow module extension usages to be isolated (#18727)
ac0f830 
If `isolate = True` is specified on `use_extension`, that particular usage will be isolated from all other usages, both in the same and other modules.

Module extensions can check whether they are isolated (e.g. in case they can only be used in this way) via `module_ctx.is_isolated`.

Closes #18529.

PiperOrigin-RevId: 541823020
Change-Id: I68a7b49914bbc1fd50df2fda7a0af1e47421bb92

Co-authored-by: Fabian Meumertzheim <fabian@meumertzhe.im>
@ylecornec ylecornec mentioned this pull request Jun 30, 2023
Merged
@vladmos vladmos mentioned this pull request Jul 11, 2023
Merged
@aherrmann aherrmann mentioned this pull request Sep 18, 2023
Closed
@avdv avdv mentioned this pull request Sep 20, 2023
Open
aherrmann added a commit to tweag/rules_nixpkgs that referenced this pull request Sep 29, 2023
@aherrmann
Implement a simplified form of nix_repo
ebfed29 
This uses the new isolated module extensions feature [1], instead of a
hub-repository approach [2] [3], to distinguish globally unified
repositories and locally specialized repositories.

[1]: bazelbuild/bazel#18529
[2]: bazelbuild/bazel#17048
[3]: bazelbuild/bazel#17493
@aherrmann aherrmann mentioned this pull request Sep 29, 2023
Merged
@aherrmann aherrmann mentioned this pull request Feb 23, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aherrmann aherrmann aherrmann left review comments

@Wyverald Wyverald Wyverald approved these changes

@meteorcloudy meteorcloudy Awaiting requested review from meteorcloudy meteorcloudy is a code owner

Assignees
No one assigned
Labels
team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@fmeum @SalmaSamy @meteorcloudy @Wyverald @aherrmann @bazel-io @sgowroji