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

Add support for overlays in source.json #22349

Closed
wants to merge 13 commits into from
Closed

Add support for overlays in source.json #22349

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
7fa7d37
Add support for remote_files to bzlmod
fzakaria May 13, 2024
3288fc8
Addressed feedback fmeum@
fzakaria May 14, 2024
858b911
add static import for collectors
fzakaria May 14, 2024
ece6861
Fix mod_command_test.py
fzakaria May 14, 2024
25a4869
Stylistic changes and GSON update
fmeum May 14, 2024
8a4930f
Merge pull request #1 from fmeum/review
fzakaria May 14, 2024
4946611
Merge branch 'master' into bcr-overlay
fzakaria May 14, 2024
2afc01a
Update lock file
fzakaria May 14, 2024
fa81a39
Merge branch 'master' into bcr-overlay
fzakaria May 15, 2024
e7f58e3
Changed overlay setup
fzakaria May 15, 2024
a1e6791
Refactor creation of mirror urls into helper method
fzakaria May 15, 2024
1a2d3ca
Remove mirror from overlay
fzakaria May 15, 2024
3e27cce
Feedback from Wyverald addressed
fzakaria May 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion MODULE.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ maven.install(
"com.google.auto.value:auto-value:1.8.2",
"com.google.auto.value:auto-value-annotations:1.9",
"com.google.code.findbugs:jsr305:3.0.2",
"com.google.code.gson:gson:2.9.0",
"com.google.code.gson:gson:2.10.1",
"com.google.code.java-allocation-instrumenter:java-allocation-instrumenter:3.3.0",
"com.google.errorprone:error_prone_annotation:2.23.0",
"com.google.errorprone:error_prone_annotations:2.23.0",
Expand Down
23 changes: 6 additions & 17 deletions MODULE.bazel.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 5 additions & 5 deletions maven_install.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
{
"__AUTOGENERATED_FILE_DO_NOT_MODIFY_THIS_FILE_MANUALLY": "THERE_IS_NO_DATA_ONLY_ZUUL",
"__INPUT_ARTIFACTS_HASH": -1536314734,
"__RESOLVED_ARTIFACTS_HASH": -1123103492,
"__INPUT_ARTIFACTS_HASH": 1275969355,
"__RESOLVED_ARTIFACTS_HASH": -209014774,
"conflict_resolution": {
"com.google.auto.value:auto-value-annotations:1.9": "com.google.auto.value:auto-value-annotations:1.10.4",
"com.google.code.gson:gson:2.8.9": "com.google.code.gson:gson:2.9.0",
"com.google.code.gson:gson:2.8.9": "com.google.code.gson:gson:2.10.1",
"com.google.errorprone:error_prone_annotations:2.23.0": "com.google.errorprone:error_prone_annotations:2.24.1",
"com.google.errorprone:error_prone_annotations:2.3.2": "com.google.errorprone:error_prone_annotations:2.24.1",
"com.google.guava:failureaccess:1.0.1": "com.google.guava:failureaccess:1.0.2",
Expand Down Expand Up @@ -122,9 +122,9 @@
},
"com.google.code.gson:gson": {
"shasums": {
"jar": "c96d60551331a196dac54b745aa642cd078ef89b6f267146b705f2c2cbef052d"
"jar": "4241c14a7727c34feea6507ec801318a3d4a90f070e4525681079fb94ee4c593"
},
"version": "2.9.0"
"version": "2.10.1"
},
"com.google.code.java-allocation-instrumenter:java-allocation-instrumenter": {
"shasums": {
Expand Down
28 changes: 28 additions & 0 deletions src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ArchiveRepoSpecBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,17 @@

package com.google.devtools.build.lib.bazel.bzlmod;

import static java.util.stream.Collectors.toList;
import static java.util.stream.Collectors.toMap;

import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
import java.net.URL;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import net.starlark.java.eval.StarlarkInt;

/**
Expand Down Expand Up @@ -77,6 +84,21 @@ public ArchiveRepoSpecBuilder setRemotePatches(ImmutableMap<String, String> remo
return this;
}

@CanIgnoreReturnValue
public ArchiveRepoSpecBuilder setOverlay(ImmutableMap<String, RemoteFile> overlay) {
Map<String, List<String>> remoteFiles =
overlay.entrySet().stream()
.collect(
toMap(
fzakaria marked this conversation as resolved.
Show resolved Hide resolved
Entry::getKey,
e -> e.getValue().urls().stream().map(URL::toString).collect(toList())));
Map<String, String> remoteFilesIntegrity =
overlay.entrySet().stream().collect(toMap(Entry::getKey, e -> e.getValue().integrity()));
attrBuilder.put("remote_file_urls", remoteFiles);
attrBuilder.put("remote_file_integrity", remoteFilesIntegrity);
return this;
}

@CanIgnoreReturnValue
public ArchiveRepoSpecBuilder setRemotePatchStrip(int remotePatchStrip) {
attrBuilder.put("remote_patch_strip", StarlarkInt.of(remotePatchStrip));
Expand All @@ -98,4 +120,10 @@ public RepoSpec build() {
.setAttributes(AttributeValues.create(attrBuilder.buildOrThrow()))
.build();
}

/**
* A simple pojo to track remote files that are offered at multiple urls (mirrors) with a single
* integrity. We split up the file here to simplify the dependency.
*/
public record RemoteFile(String integrity, List<URL> urls) {}
}
5 changes: 5 additions & 0 deletions src/main/java/com/google/devtools/build/lib/bazel/bzlmod/IndexRegistry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,7 @@ private static class ArchiveSourceJson {
String integrity;
String stripPrefix;
Map<String, String> patches;
Map<String, ArchiveRepoSpecBuilder.RemoteFile> overlay;
int patchStrip;
String archiveType;
}
Expand Down Expand Up @@ -391,6 +392,10 @@ private RepoSpec createArchiveRepoSpec(
.setIntegrity(sourceJson.integrity)
.setStripPrefix(Strings.nullToEmpty(sourceJson.stripPrefix))
.setRemotePatches(remotePatches.buildOrThrow())
.setOverlay(
sourceJson.overlay != null
? ImmutableMap.copyOf(sourceJson.overlay)
: ImmutableMap.of())
.setRemotePatchStrip(sourceJson.patchStrip)
.setArchiveType(sourceJson.archiveType)
.build();
Expand Down
43 changes: 43 additions & 0 deletions src/test/java/com/google/devtools/build/lib/bazel/bzlmod/IndexRegistryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import java.io.File;
import java.io.IOException;
import java.io.Writer;
import java.net.URI;
import java.nio.file.Files;
import java.util.ArrayList;
import java.util.List;
Expand Down Expand Up @@ -182,6 +183,23 @@ public void testGetArchiveRepoSpec() throws Exception {
" },",
" \"patch_strip\": 3",
"}");
server.serve(
"/modules/baz/3.0/source.json",
"""
{
"url": "https://example.com/archive.jar?with=query",
"integrity": "sha256-bleh",
"overlay": {
"BUILD.bazel": {
"urls": [
"http://mirror1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Wyverald One thing I'm wondering about right now: There is no such thing for remote patches, but should we use the mirrors in bazel_registry.json to populate these URLs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could see from the test is that mirrors is only used for the main URL of the module.
I also had the same thought that it was a bit confusing --

Since this is a distinct URL it actually doesn't even need to reside within BCR server so it probably needs it's own URLS.

in the simple/common case, it could be a relative file path and just use a single URL.

Maybe a follow up: if the URL is a file:// use the bazel_registry.json mirrors?

I say let's finish the complete end-to-end with BCR first and then do a UX fixup once we test it out.

Copy link
Member

Choose a reason for hiding this comment

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

taking a more serious look now -- shouldn't the overlay files be served from the registry itself? Just like how today patch files are served from the registry, instead of some random other URL. The only thing we're fetching from arbitrary URLs is the source archive itself.

I'm thinking that the overlay property can just be a JSON object (i.e. a dict) that maps from overlay file path to integrity (just like how patches is a map from patch file name to integrity). The overlay files will reside in the overlay directory next to the patches directory. So if you have the source.json:

{
    "url": "...",
    "integrity": "...",
    "overlay": {
        "BUILD.bazel": "sha256-...",
        "mypkg/BUILD.bazel": "sha256-..."
    }
}

then your module version directory tree in the registry should look like:

|-- MODULE.bazel
|-- source.json
|-- overlay
    |-- BUILD.bazel
    |-- mypkg
        |-- BUILD.bazel

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I see this and construct the URL similar to how it's done for patches?

"http://mirror2"
],
"integrity": "sha256-bleh-overlay"
}
}
}
""");
server.start();

Registry registry =
Expand All @@ -198,6 +216,7 @@ public void testGetArchiveRepoSpec() throws Exception {
.setIntegrity("sha256-blah")
.setStripPrefix("pref")
.setRemotePatches(ImmutableMap.of())
.setOverlay(ImmutableMap.of())
.setRemotePatchStrip(0)
.build());
assertThat(registry.getRepoSpec(createModuleKey("bar", "2.0"), reporter))
Expand All @@ -216,6 +235,28 @@ public void testGetArchiveRepoSpec() throws Exception {
server.getUrl() + "/modules/bar/2.0/patches/2.fix-that.patch",
"sha256-kek"))
.setRemotePatchStrip(3)
.setOverlay(ImmutableMap.of())
.build());
assertThat(registry.getRepoSpec(createModuleKey("baz", "3.0"), reporter))
.isEqualTo(
new ArchiveRepoSpecBuilder()
.setUrls(
ImmutableList.of(
"https://mirror.bazel.build/example.com/archive.jar?with=query",
"file:///home/bazel/mymirror/example.com/archive.jar?with=query",
"https://example.com/archive.jar?with=query"))
.setIntegrity("sha256-bleh")
.setStripPrefix("")
.setOverlay(
ImmutableMap.of(
"BUILD.bazel",
new ArchiveRepoSpecBuilder.RemoteFile(
"sha256-bleh-overlay",
ImmutableList.of(
URI.create("http://mirror1").toURL(),
URI.create("http://mirror2").toURL()))))
.setRemotePatches(ImmutableMap.of())
.setRemotePatchStrip(0)
.build());
}

Expand Down Expand Up @@ -264,6 +305,7 @@ public void testGetRepoInvalidRegistryJsonSpec() throws Exception {
.setIntegrity("sha256-blah")
.setStripPrefix("pref")
.setRemotePatches(ImmutableMap.of())
.setOverlay(ImmutableMap.of())
.setRemotePatchStrip(0)
.build());
}
Expand Down Expand Up @@ -351,6 +393,7 @@ public void testArchiveWithExplicitType() throws Exception {
.setArchiveType("zip")
.setRemotePatches(ImmutableMap.of())
.setRemotePatchStrip(0)
.setOverlay(ImmutableMap.of())
.build());
}

Expand Down
34 changes: 20 additions & 14 deletions src/test/py/bazel/bzlmod/mod_command_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,13 +397,15 @@ def testShowModuleAndExtensionReposFromBaseModule(self):
)
self.assertRegex(stdout.pop(4), r'^ urls = \[".*"\],$')
self.assertRegex(stdout.pop(4), r'^ integrity = ".*",$')
stdout.pop(11)
self.assertRegex(stdout.pop(16), r'^ path = ".*",$')
stdout.pop(29)
stdout.pop(39)
self.assertRegex(stdout.pop(44), r'^ urls = \[".*"\],$')
self.assertRegex(stdout.pop(44), r'^ integrity = ".*",$')
stdout.pop(51)
self.assertRegex(stdout.pop(19), r'^ path = ".*",$')
# lines after 'Rule data_repo defined at (most recent call last):'
stdout.pop(32)
stdout.pop(42)
self.assertRegex(stdout.pop(47), r'^ urls = \[".*"\],$')
self.assertRegex(stdout.pop(47), r'^ integrity = ".*",$')
# lines after '# Rule http_archive defined at (most recent call last):'
stdout.pop(13)
stdout.pop(55)
self.assertListEqual(
stdout,
[
Expand All @@ -414,19 +416,21 @@ def testShowModuleAndExtensionReposFromBaseModule(self):
# pop(4) -- urls=[...]
# pop(4) -- integrity=...
' strip_prefix = "",',
' remote_file_urls = {},',
' remote_file_integrity = {},',
' remote_patches = {},',
' remote_patch_strip = 0,',
')',
'# Rule bar~ instantiated at (most recent call last):',
'# <builtin> in <toplevel>',
'# Rule http_archive defined at (most recent call last):',
# pop(11)
# pop(13)
'',
'## ext@1.0:',
'# <builtin>',
'local_repository(',
' name = "ext~",',
# pop(16) -- path=...
# pop(19) -- path=...
')',
'# Rule ext~ instantiated at (most recent call last):',
'# <builtin> in <toplevel>',
Expand All @@ -440,7 +444,7 @@ def testShowModuleAndExtensionReposFromBaseModule(self):
'# Rule ext~~ext~repo3 instantiated at (most recent call last):',
'# <builtin> in <toplevel>',
'# Rule data_repo defined at (most recent call last):',
# pop(29)
# pop(32)
'',
'## @my_repo4:',
'# <builtin>',
Expand All @@ -451,22 +455,24 @@ def testShowModuleAndExtensionReposFromBaseModule(self):
'# Rule ext~~ext~repo4 instantiated at (most recent call last):',
'# <builtin> in <toplevel>',
'# Rule data_repo defined at (most recent call last):',
# pop(39)
# pop(42)
'',
'## bar@2.0:',
'# <builtin>',
'http_archive(',
' name = "bar~",',
# pop(44) -- urls=[...]
# pop(44) -- integrity=...
# pop(47) -- urls=[...]
# pop(47) -- integrity=...
' strip_prefix = "",',
' remote_file_urls = {},',
' remote_file_integrity = {},',
' remote_patches = {},',
' remote_patch_strip = 0,',
')',
'# Rule bar~ instantiated at (most recent call last):',
'# <builtin> in <toplevel>',
'# Rule http_archive defined at (most recent call last):',
# pop(51)
# pop(55)
'',
],
'wrong output in the show query for module and extension-generated'
Expand Down