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

bazel_7: 7.1.0 -> 7.1.2 #314279

Merged
merged 3 commits into from
Jun 3, 2024
Merged

bazel_7: 7.1.0 -> 7.1.2 #314279

merged 3 commits into from
Jun 3, 2024

Conversation

malt3
Copy link
Contributor

@malt3 malt3 commented May 24, 2024

Description of changes

Upgrade Bazel to 7.1.2 and make parsing of MODULE.bazel.lock compatible with lockFileVersion 4, 5, and 6.
Also pinging @Strum355. For some reason, I am unable to add you as a reviewer.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

then addSources acc
else acc;

requiredSourcePredicate = n: requiredDepNamePredicate (sanitize n);
requiredDeps = foldlJSON (extract_source requiredSourcePredicate) { } modules;
requiredDeps = foldlModuleDepGraph (extract_source requiredSourcePredicate) { } modules // foldlGeneratedRepoSpecs (extract_source requiredSourcePredicate) { } modules;
Copy link
Contributor Author

@malt3 malt3 May 24, 2024

Choose a reason for hiding this comment

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

I'm not sure about the naming "foldl..." here. Also, this looks a bit verbose. Suggestions welcome.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, parsing twice is not ideal, but parsing at all may not survive 7.2.0, so good enough for now.

Comment on lines -27 to +28
null == builtins.match ".*(macos|osx|linux|win|apple|android|maven).*" name
&& null != builtins.match "(platforms|com_google_|protobuf|rules_|bazel_).*" name ;
null == builtins.match ".*(macos|osx|linux|win|android|maven).*" name
&& null != builtins.match "(platforms|com_google_|protobuf|rules_|bazel_|apple_support).*" name;
Copy link
Contributor Author

@malt3 malt3 May 24, 2024

Choose a reason for hiding this comment

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

This might no longer filter all platform specific deps with apple in the name. I needed this to allow downloading of apple_support. Suggestions welcome.

Copy link
Member

Choose a reason for hiding this comment

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

Having too much here is not a big issue. It bloats the build closure, but not the runtime closure. So pretty innocuous.

Copy link
Contributor

@Strum355 Strum355 left a comment

Choose a reason for hiding this comment

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

Tested on x86_64-linux and seems to work fine 👍 can't give an official stamp as a non-member

@olebedev olebedev removed their request for review May 30, 2024 09:09
The new lockfile format 6 drops the "name" attribute of each repoSpec.
See also: bazelbuild/bazel#21026
This prevents the builder from effectively the deps using
requiredDepNamePredicate.
Instead, we now generate names from other metadata.
@ofborg ofborg bot requested a review from olebedev May 30, 2024 17:58
@malt3
Copy link
Contributor Author

malt3 commented May 31, 2024

I'm also playing around with the RCs of 7.2.0 and noticed that we may need to find a different approach (stop parsing MODULE.bazel.lock), since it no longer contains all the required info.

Some module extensions no longer show up in the lockfile if they are marked as reproducible (example, upstream change). Also, some required external dependencies are now recorded in source.json files that are hash pinned in the lockfile: bazelbuild/bazel#22296
This means we would have to vendor all referenced source.json files in nixpkgs (to work around import from derivation).

I would like to discuss alternatives in some appropriate channel. Any suggestions? Probably either a forum or matrix channel.

My ideas for solving this are:

  • run bazel fetch / vendor / build outside the sandbox (with internet access) and record what http requests are made (using --experimental_remote_downloader with a custom-built download proxy that logs all requests) and create our own list of dependencies. Commit this dependency list in nixpkgs (similar to src-deps.json in bazel_6 and older). This solution allows for fine-grained deduplication, since we can still generate individual fetchurl derivations.
  • run bazel fetch / vendor as part of a fixed output derivation that consists of the repository cache (the vendor command is currently being reworked to ensure it is good enough to download everything required for air-gapped builds: 1, 2). This solution is not fine-grained by default, since it creates one large derivation containing all dependencies and thus preventing deduplication. On the plus side, this may reduce the maintenance burden within nixpkgs, since the vendor mode is an advertised feature of bazel.

@malt3
Copy link
Contributor Author

malt3 commented May 31, 2024

Result of nixpkgs-review pr 314279 run on x86_64-linux 1

1 package built:
  • bazel_7

Copy link
Member

@layus layus left a comment

Choose a reason for hiding this comment

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

LGTM.

then addSources acc
else acc;

requiredSourcePredicate = n: requiredDepNamePredicate (sanitize n);
requiredDeps = foldlJSON (extract_source requiredSourcePredicate) { } modules;
requiredDeps = foldlModuleDepGraph (extract_source requiredSourcePredicate) { } modules // foldlGeneratedRepoSpecs (extract_source requiredSourcePredicate) { } modules;
Copy link
Member

Choose a reason for hiding this comment

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

Yes, parsing twice is not ideal, but parsing at all may not survive 7.2.0, so good enough for now.

Comment on lines -27 to +28
null == builtins.match ".*(macos|osx|linux|win|apple|android|maven).*" name
&& null != builtins.match "(platforms|com_google_|protobuf|rules_|bazel_).*" name ;
null == builtins.match ".*(macos|osx|linux|win|android|maven).*" name
&& null != builtins.match "(platforms|com_google_|protobuf|rules_|bazel_|apple_support).*" name;
Copy link
Member

Choose a reason for hiding this comment

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

Having too much here is not a big issue. It bloats the build closure, but not the runtime closure. So pretty innocuous.

@layus layus merged commit c429fa2 into NixOS:master Jun 3, 2024
28 checks passed
@malt3 malt3 deleted the bazel-7-1-1 branch June 3, 2024 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants