Add support for overlays in source.json #22349

fzakaria · 2024-05-13T18:09:49Z

This is a continuation of #22155 that adds the newly added 'remote_files' attribute for http_archive to the bzlmod functionality.

The end goal is to then update BCR to this new functionality to overlay files rather than use patch files when providing MODULE/WORKSPACE/BUILD files.

bazelbuild/bazel-central-registry#1566 has a good discussion of the rationale.

Co-authored-by: Fabian Meumertzheim fabian@meumertzhe.im

This is a continuation of bazelbuild#22155 that adds the newly added 'remote_files' attribute for http_archive to the bzlmod functionality. The end goal is to then update BCR to this new functionality to overlay files rather than use patch files when providing MODULE/WORKSPACE/BUILD files. bazelbuild/bazel-central-registry#1566 has a good discussion of the rationale.

fzakaria · 2024-05-13T18:11:24Z

src/test/java/com/google/devtools/build/lib/bazel/bzlmod/IndexRegistryTest.java

+        "/modules/baz/3.0/source.json",
+        "{",
+        "  \"url\": \"https://example.com/archive.jar?with=query\",",
+        "  \"integrity\": \"sha256-bleh\",",
+        "  \"overlay\": {",
+        "    \"BUILD.bazel\": {",
+        "       \"urls\": [\"http://mirror1\", \"http://mirror2\"],",
+        "       \"integrity\": \"sha256-bleh-overlay\"",
+        "   }",
+        "  }",
+        "}");


I chose a structure (overlay) slightly different than the http_archive to simplify it.
http_archive is restricted to certain attr type (I.e str_dict_list) but in JSON we can be a lot more ergonomic to combine the integrity and the mirror URLS.

just driving by -- I think this is a great idea!

fmeum · 2024-05-13T19:36:34Z

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RemoteFile.java

+ * offered at multiple urls (mirrors) with a single integrity.
+ * We split up the file here to simplify the dependency.
+ */
+class RemoteFile {


Make this a record, then it's short enough to be moved into IndexRegistry as an inner class.

I can't move it within IndexRegistry because I use it in ArchiveRepoSpecBuilder ; it would cause circular reference.
I can move it into ArchiveRepoSpecBuilder or keep as separate file?

I changed to record -- I wasn't aware Bazel is on a new JDK.
(How can I tell the version?)

I also had to write a custom serializer since GSON can't set the record fields as they were final
(I commented on the rationale in the code)

Bazel uses Java/JDK 21. You can look for --java_language_version in .bazelrc. GSON 2.10.1 has support for records.

Cool -- I changed one of the tests to use Text blocks also to simplify the JSON processing;
Yay for new Java.

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/IndexRegistry.java

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ArchiveRepoSpecBuilder.java

fmeum · 2024-05-13T19:40:28Z

The failing test (testShowModuleAndExtensionReposFromBaseModule) is pretty sensitive to any kind of changes to the spec builders, so you might have to adjust the number of skipped lines. Let me know if you have problems running the test to capture the full output.

fzakaria · 2024-05-14T00:22:48Z

The failing test (testShowModuleAndExtensionReposFromBaseModule) is pretty sensitive to any kind of changes to the spec builders, so you might have to adjust the number of skipped lines. Let me know if you have problems running the test to capture the full output.

I printed the output but I'm not sure exactly which fields the test wants to validate.
Here is the stdout pretty printed:

['## @bar_from_foo2:',
 '# <builtin>',
 'http_archive(',
 '  name = "bar~",',
 '  urls = '
 '["file:/tmp/bazel-working-directory/_main/_tmp/95ebb5496eb54e209206860635685b44/tests_root/tmp9dqgcyja/tmplpgvxv_9/main/archives/bar.2.0.zip"],',
 '  integrity = "sha256-j3wLXYtBrK/WDy97QCU3oI1HB2+0AYPHyjO6mBlbAVw=",',
 '  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):',
 '#   '
 '/tmp/bazel-working-directory/_main/_tmp/95ebb5496eb54e209206860635685b44/_bazel_fmzakari/627ca31f67019a002c2b9671f4bd938d/external/bazel_tools/tools/build_defs/repo/http.bzl:397:31 '
 'in <toplevel>',
 '',
 '## ext@1.0:',
 '# <builtin>',
 'local_repository(',
 '  name = "ext~",',
 '  path = '
 '"/tmp/bazel-working-directory/_main/_tmp/95ebb5496eb54e209206860635685b44/tests_root/tmp9dqgcyja/tmplpgvxv_9/main/projects/ext",',
 ')',
 '# Rule ext~ instantiated at (most recent call last):',
 '#   <builtin> in <toplevel>',
 '',
 '## @my_repo3:',
 '# <builtin>',
 'data_repo(',
 '  name = "ext~~ext~repo3",',
 '  data = "requested repo",',
 ')',
 '# Rule ext~~ext~repo3 instantiated at (most recent call last):',
 '#   <builtin> in <toplevel>',
 '# Rule data_repo defined at (most recent call last):',
 '#   '
 '/tmp/bazel-working-directory/_main/_tmp/95ebb5496eb54e209206860635685b44/_bazel_fmzakari/627ca31f67019a002c2b9671f4bd938d/external/ext~/ext.bzl:2:28 '
 'in <toplevel>',
 '',
 '## @my_repo4:',
 '# <builtin>',
 'data_repo(',
 '  name = "ext~~ext~repo4",',
 '  data = "requested repo",',
 ')',
 '# Rule ext~~ext~repo4 instantiated at (most recent call last):',
 '#   <builtin> in <toplevel>',
 '# Rule data_repo defined at (most recent call last):',
 '#   '
 '/tmp/bazel-working-directory/_main/_tmp/95ebb5496eb54e209206860635685b44/_bazel_fmzakari/627ca31f67019a002c2b9671f4bd938d/external/ext~/ext.bzl:2:28 '
 'in <toplevel>',
 '',
 '## bar@2.0:',
 '# <builtin>',
 'http_archive(',
 '  name = "bar~",',
 '  urls = '
 '["file:/tmp/bazel-working-directory/_main/_tmp/95ebb5496eb54e209206860635685b44/tests_root/tmp9dqgcyja/tmplpgvxv_9/main/archives/bar.2.0.zip"],',
 '  integrity = "sha256-j3wLXYtBrK/WDy97QCU3oI1HB2+0AYPHyjO6mBlbAVw=",',
 '  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):',
 '#   '
 '/tmp/bazel-working-directory/_main/_tmp/95ebb5496eb54e209206860635685b44/_bazel_fmzakari/627ca31f67019a002c2b9671f4bd938d/external/bazel_tools/tools/build_defs/repo/http.bzl:397:31 '
 'in <toplevel>',
 '']

fzakaria · 2024-05-14T03:36:23Z

Okay it should be passing now. That was a miserable test to update.

Target //src/test/py/bazel:mod_command_test up-to-date:
  bazel-bin/src/test/py/bazel/mod_command_test
INFO: Elapsed time: 11.439s, Critical Path: 11.14s
INFO: 2 processes: 1 internal, 1 linux-sandbox.
INFO: Build completed successfully, 2 total actions
//src/test/py/bazel:mod_command_test

fmeum

I have a few stylistic suggestions as well as a GSON update for record support at fzakaria#1, please take a look.

Otherwise LGTM.

Stylistic changes and GSON update

fzakaria · 2024-05-14T16:21:52Z

I have a few stylistic suggestions as well as a GSON update for record support at fzakaria#1, please take a look.

Otherwise LGTM.

Done -- thank you for the contribution to simplify the process.

fmeum

@Wyverald Could you review for approval?

fmeum · 2024-05-14T18:03:46Z

src/test/java/com/google/devtools/build/lib/bazel/bzlmod/IndexRegistryTest.java

+            "overlay": {
+                "BUILD.bazel": {
+                    "urls": [
+                        "http://mirror1",


@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?

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.

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?

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

* Used mirrors from bazel_registry.json to determine mirrors * Changed overlay in source.json to be more simple

fzakaria · 2024-05-15T18:43:07Z

@Wyverald I believe I've made the changes requested to simplify the archive JSON structure.

fzakaria · 2024-05-15T18:43:27Z

Ah -- I closed it by accident :(

I also updated the code as required @Wyverald

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ArchiveRepoSpecBuilder.java

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/IndexRegistry.java

src/test/java/com/google/devtools/build/lib/bazel/bzlmod/IndexRegistryTest.java

fmeum · 2024-05-16T08:16:28Z

@bazel-io fork 7.2.0

This is a continuation of bazelbuild#22155 that adds the newly added 'remote_files' attribute for http_archive to the bzlmod functionality. The end goal is to then update BCR to this new functionality to overlay files rather than use patch files when providing MODULE/WORKSPACE/BUILD files. bazelbuild/bazel-central-registry#1566 has a good discussion of the rationale. Co-authored-by: Fabian Meumertzheim <fabian@meumertzhe.im> Closes bazelbuild#22349. PiperOrigin-RevId: 636682112 Change-Id: Ief070985598a7c0f427a98cd3daeb69a0984f7be

This is a continuation of #22155 that adds the newly added 'remote_files' attribute for http_archive to the bzlmod functionality. The end goal is to then update BCR to this new functionality to overlay files rather than use patch files when providing MODULE/WORKSPACE/BUILD files. bazelbuild/bazel-central-registry#1566 has a good discussion of the rationale. Co-authored-by: Fabian Meumertzheim <fabian@meumertzhe.im> Closes #22349. PiperOrigin-RevId: 636682112 Change-Id: Ief070985598a7c0f427a98cd3daeb69a0984f7be Commit c4167e3 Co-authored-by: Farid Zakaria <fmzakari@google.com>

meteorcloudy · 2024-07-22T13:14:37Z

Sorry for missing this in the original review, but we should add documentation for this feature at https://bazel.build/external/registry

fzakaria · 2024-07-22T14:22:05Z

@meteorcloudy we never landed the registry support bazelbuild/bazel-central-registry#2046

I think last place we landed was how to enable it in the registry and assume version running.
Do we want to make modules that are the "new overlay format" only now?

Happy to finish this off given some direction & agreement.

meteorcloudy · 2024-07-22T14:34:44Z

@fzakaria The bcr_validation.py was fixed in bazelbuild/bazel-central-registry#2249 and the BCR presubmit now supports overlay files after bazelbuild/continuous-integration#2004.

Even without the above changes, since the change in the PR works with any Bazel registry (not just BCR), it should still be documented. ;)

fzakaria requested review from Wyverald and meteorcloudy as code owners May 13, 2024 18:09

github-actions bot added team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. awaiting-review PR is awaiting review from an assigned reviewer labels May 13, 2024

fzakaria commented May 13, 2024

View reviewed changes

fmeum reviewed May 13, 2024

View reviewed changes

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/IndexRegistry.java Outdated Show resolved Hide resolved

fmeum reviewed May 13, 2024

View reviewed changes

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ArchiveRepoSpecBuilder.java Outdated Show resolved Hide resolved

fzakaria added 2 commits May 14, 2024 00:20

Addressed feedback fmeum@

3288fc8

add static import for collectors

858b911

Fix mod_command_test.py

ece6861

fzakaria requested a review from fmeum May 14, 2024 03:36

Stylistic changes and GSON update

25a4869

fmeum reviewed May 14, 2024

View reviewed changes

Merge pull request #1 from fmeum/review

8a4930f

Stylistic changes and GSON update

fzakaria added 2 commits May 14, 2024 16:28

Merge branch 'master' into bcr-overlay

4946611

Update lock file

2afc01a

fzakaria requested a review from fmeum May 14, 2024 16:29

fmeum approved these changes May 14, 2024

View reviewed changes

fmeum reviewed May 14, 2024

View reviewed changes

fzakaria added 4 commits May 15, 2024 16:36

Merge branch 'master' into bcr-overlay

fa81a39

Changed overlay setup

e7f58e3

* Used mirrors from bazel_registry.json to determine mirrors * Changed overlay in source.json to be more simple

Refactor creation of mirror urls into helper method

a1e6791

Remove mirror from overlay

1a2d3ca

fzakaria requested a review from fmeum May 15, 2024 18:36

fzakaria closed this May 15, 2024

fzakaria reopened this May 15, 2024

github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label May 15, 2024

Wyverald reviewed May 15, 2024

View reviewed changes

Feedback from Wyverald addressed

3e27cce

Wyverald approved these changes May 15, 2024

View reviewed changes

Wyverald added the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label May 15, 2024

Wyverald changed the title ~~Add support for remote_files to bzlmod~~ Add support for overlays in source.json May 15, 2024

bazel-io mentioned this pull request May 16, 2024

[7.2.0] Add support for overlays in source.json #22396

Closed

fzakaria mentioned this pull request May 16, 2024

Add support for overlay in source.json bazelbuild/bazel-central-registry#2046

Closed

copybara-service bot closed this in c4167e3 May 23, 2024

github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label May 23, 2024

bazel-io mentioned this pull request May 23, 2024

[7.2.0] Add support for overlays in source.json #22525

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for overlays in source.json #22349

Add support for overlays in source.json #22349

fzakaria commented May 13, 2024 •

edited

Loading

fzakaria May 13, 2024

Wyverald May 13, 2024

fmeum May 13, 2024

fzakaria May 14, 2024

fmeum May 14, 2024

fzakaria May 14, 2024

fmeum commented May 13, 2024

fzakaria commented May 14, 2024

fzakaria commented May 14, 2024

fmeum left a comment

fzakaria commented May 14, 2024

fmeum left a comment

fmeum May 14, 2024

fzakaria May 14, 2024

Wyverald May 14, 2024

fzakaria May 14, 2024

fzakaria commented May 15, 2024

fzakaria commented May 15, 2024

fmeum commented May 16, 2024

meteorcloudy commented Jul 22, 2024

fzakaria commented Jul 22, 2024

meteorcloudy commented Jul 22, 2024

Add support for overlays in source.json #22349

Add support for overlays in source.json #22349

Conversation

fzakaria commented May 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmeum commented May 13, 2024

fzakaria commented May 14, 2024

fzakaria commented May 14, 2024

fmeum left a comment

Choose a reason for hiding this comment

fzakaria commented May 14, 2024

fmeum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fzakaria commented May 15, 2024

fzakaria commented May 15, 2024

fmeum commented May 16, 2024

meteorcloudy commented Jul 22, 2024

fzakaria commented Jul 22, 2024

meteorcloudy commented Jul 22, 2024

fzakaria commented May 13, 2024 •

edited

Loading