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

Add override_repo and inject_repo #23534

Closed
wants to merge 19 commits into from
Closed

Add override_repo and inject_repo #23534

wants to merge 19 commits into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Sep 6, 2024 •
edited
Loading

Work towards #19301
Fixes #23580

RELNOTES: override_repo and inject_repo can be used to override and inject repos in module extensions.

@github-actions github-actions bot added team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. team-Documentation Documentation improvements that cannot be directly linked to other team labels awaiting-review PR is awaiting review from an assigned reviewer labels Sep 6, 2024
@fmeum fmeum force-pushed the 19301-override-repo branch 2 times, most recently from 71e6ce0 to 0ca3ea0 Compare September 6, 2024 14:08
Wyverald
Wyverald reviewed Sep 10, 2024
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.

Overall looks great! I haven't finished reviewing yet (mostly just looked at ModuleFileGlobals & ModuleThreadContext); will continue later.

site/en/external/extension.md Outdated Show resolved Hide resolved
src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java Outdated Show resolved Hide resolved
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/ModuleThreadContext.java Outdated Show resolved Hide resolved
src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java Outdated Show resolved Hide resolved
src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java Outdated Show resolved Hide resolved
src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java Outdated Show resolved Hide resolved
src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java Outdated Show resolved Hide resolved
src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java Outdated Show resolved Hide resolved
@fmeum fmeum mentioned this pull request Sep 10, 2024
Closed
@fmeum fmeum force-pushed the 19301-override-repo branch 2 times, most recently from 541c766 to 8b07c7c Compare September 15, 2024 11:34
@fmeum
Copy link
Collaborator Author

fmeum commented Sep 15, 2024

As I still see many folks specify only a single repo per use_repo, I wonder whether we should use the plural form for inject_repo and override_repo.

@fmeum fmeum requested a review from Wyverald September 15, 2024 11:48
@fmeum fmeum changed the title Add override_repo Add override_repo and inject_repo Sep 15, 2024
Wyverald
Wyverald reviewed Sep 16, 2024
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.

haven't looked at tests yet, but this overall looks good to me. Thanks for getting this in place!

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java Outdated Show resolved Hide resolved
src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java Outdated Show resolved Hide resolved
src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java Outdated Show resolved Hide resolved
src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java Outdated Show resolved Hide resolved
src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java Outdated Show resolved Hide resolved
src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionUsage.java Outdated Show resolved Hide resolved
src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionUsagesValue.java Show resolved Hide resolved
src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphValue.java Outdated Show resolved Hide resolved
src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunction.java Outdated Show resolved Hide resolved
src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunction.java Outdated Show resolved Hide resolved
@fmeum fmeum force-pushed the 19301-override-repo branch 2 times, most recently from 53b0fd2 to 16ff6bc Compare September 19, 2024 16:53
@fmeum
Copy link
Collaborator Author

fmeum commented Sep 19, 2024

Why does the fact that overriding repos cannot be overridden mean "overrides do need to applied recursively"? What does applying an override recursively even mean?

I removed this comment, it's actually no longer relevant at that point since the overriding repo has already been resolved to a canonical name. I was referring to the "no chains" requirement relevant in BazelDepGraphFunction#resolveRepoOverrides, but you are totally right about this not mattering at the location you commented on.

@fmeum fmeum requested a review from Wyverald September 19, 2024 20:28
Wyverald
Wyverald approved these changes Sep 19, 2024
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.

🚀!!

@Wyverald
Copy link
Member

Hmm. I have no idea why, but command_profiler_test seems to be consistently failing. Presubmit runs for other PRs don't seem to be affected. Baffling.

@fmeum
Copy link
Collaborator Author

fmeum commented Sep 19, 2024

Hmm. I have no idea why, but command_profiler_test seems to be consistently failing. Presubmit runs for other PRs don't seem to be affected. Baffling.

That was my update to JDK 23 (oopsie...). I rebased onto master, which has the revert.

@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 Sep 19, 2024
@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 Sep 24, 2024
@fmeum fmeum deleted the 19301-override-repo branch September 24, 2024 08:56
@fmeum
Copy link
Collaborator Author

fmeum commented Sep 24, 2024

@bazel-io fork 7.4.0

@bazel-io bazel-io mentioned this pull request Sep 24, 2024
Closed
fmeum added a commit to fmeum/bazel that referenced this pull request Oct 10, 2024
@fmeum
Add override_repo and inject_repo
630b6bb 
Work towards bazelbuild#19301
Fixes bazelbuild#23580

RELNOTES: `override_repo` and `inject_repo` can be used to override and inject repos in module extensions.

Closes bazelbuild#23534.

PiperOrigin-RevId: 678139661
Change-Id: Iea7caca949c00e701f056c1037e273fee9740e93

(cherry picked from commit 46341b1)
@fmeum fmeum mentioned this pull request Oct 10, 2024
Merged
fmeum added a commit to fmeum/bazel that referenced this pull request Oct 10, 2024
@fmeum
Add override_repo and inject_repo
c074484 
Work towards bazelbuild#19301
Fixes bazelbuild#23580

RELNOTES: `override_repo` and `inject_repo` can be used to override and inject repos in module extensions.

Closes bazelbuild#23534.

PiperOrigin-RevId: 678139661
Change-Id: Iea7caca949c00e701f056c1037e273fee9740e93

(cherry picked from commit 46341b1)
fmeum added a commit to fmeum/bazel that referenced this pull request Oct 10, 2024
@fmeum
Add override_repo and inject_repo
b13ffab 
Work towards bazelbuild#19301
Fixes bazelbuild#23580

RELNOTES: `override_repo` and `inject_repo` can be used to override and inject repos in module extensions.

Closes bazelbuild#23534.

PiperOrigin-RevId: 678139661
Change-Id: Iea7caca949c00e701f056c1037e273fee9740e93

(cherry picked from commit 46341b1)
fmeum added a commit to fmeum/bazel that referenced this pull request Oct 10, 2024
@fmeum
Add override_repo and inject_repo
46369e0 
Work towards bazelbuild#19301
Fixes bazelbuild#23580

RELNOTES: `override_repo` and `inject_repo` can be used to override and inject repos in module extensions.

Closes bazelbuild#23534.

PiperOrigin-RevId: 678139661
Change-Id: Iea7caca949c00e701f056c1037e273fee9740e93

(cherry picked from commit 46341b1)
fmeum added a commit to fmeum/bazel that referenced this pull request Oct 10, 2024
@fmeum
Add override_repo and inject_repo
07938f8 
Work towards bazelbuild#19301
Fixes bazelbuild#23580

RELNOTES: `override_repo` and `inject_repo` can be used to override and inject repos in module extensions.

Closes bazelbuild#23534.

PiperOrigin-RevId: 678139661
Change-Id: Iea7caca949c00e701f056c1037e273fee9740e93

(cherry picked from commit 46341b1)
github-merge-queue bot pushed a commit that referenced this pull request Oct 10, 2024
@fmeum @Wyverald
[7.4.0] Add override_repo and inject_repo (#23938)
067ce4d 
Work towards #19301
Fixes #23580

RELNOTES: `override_repo` and `inject_repo` can be used to override and
inject repos in module extensions.

Closes #23534.

PiperOrigin-RevId: 678139661
Change-Id: Iea7caca949c00e701f056c1037e273fee9740e93

(cherry picked from commit
46341b1)

Fixes #23724
Fixes #23799

Also includes:
* Disallow importing injected repos
(8472c9d)

---------

Co-authored-by: Xùdōng Yáng <wyverald@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Wyverald Wyverald Wyverald approved these changes

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

Assignees
No one assigned
Labels
team-Documentation Documentation improvements that cannot be directly linked to other team 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.

Can't use module_override to override a repo fetched inside module extension
2 participants
@fmeum @Wyverald