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

bazel: 7.0.2 -> 7.1.0 #295615

Merged
merged 1 commit into from
Apr 25, 2024
Merged

bazel: 7.0.2 -> 7.1.0 #295615

merged 1 commit into from
Apr 25, 2024

Conversation

Strum355
Copy link
Contributor

Description of changes

Changelog: https://github.com/bazelbuild/bazel/releases/tag/7.1.0

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.05 Release Notes (or backporting 23.05 and 23.11 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.

@Strum355
Copy link
Contributor Author

@layus I don't remember what the process is for updating cpp-test-MODULE.bazel.lock and I cant find instructions on it.

@layus
Copy link
Member

layus commented Mar 13, 2024 via email

@Strum355
Copy link
Contributor Author

I enter the environment with nix-shell -A and run the process manually to update the lockfile then copy that new version. Nothing much smarter sadly. IMO we should use better projects for tests. These examples are not updated very frequently.

Not sure if I did it right, presumably not as nix-build . -A bazel_7.tests fails with errors including:

ERROR: /build/bazel_home/.cache/bazel/_bazel_nixbld/2898b59ac5a61dc07e72b28a0e3fa2ab/external/local_config_platform/BUILD.bazel:4:9: @@local_config_platform//:host depends on @@platforms//os:linux in repository @@platforms which failed to fetch. no such package '@@platforms//os': java.io.IOException: Error downloading [https://github.com/bazelbuild/platforms/releases/download/0.0.7/platforms-0.0.7.tar.gz] to /build/bazel_home/.cache/bazel/_bazel_nixbld/2898b59ac5a61dc07e72b28a0e3fa2ab/external/platforms/temp18254770786369221115/platforms-0.0.7.tar.gz: Unknown host: github.com

@Strum355 Strum355 marked this pull request as ready for review March 20, 2024 14:07
@Strum355
Copy link
Contributor Author

OfBorg seems happy with the tests, even though I couldnt get them to be successful locally 🤷 marked as ready for review https://github.com/NixOS/nixpkgs/pull/295615/checks?check_run_id=22622593892

flurie
flurie approved these changes Mar 20, 2024
View reviewed changes
Copy link
Contributor

@flurie flurie left a comment

Choose a reason for hiding this comment

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

I was able to build and run Bazel 7.1 on some real targets already up to date with 7.0.2.

I did have a different failure locally with the tests having to do with failed fetches from the bzlmod central registry. I ran it a few days ago, but I can rerun it to find the error message, or perhaps it is expected. I did not run the tests locally with the 7.0.2 upgrade, so I don't know that this PR is the culprit.

@avdv
Copy link
Member

avdv commented Mar 20, 2024

OfBorg seems happy with the tests

Well, it failed for x86 and arm64 darwin... I don't know if we should care about that?

From a quick glance, the changes look good. Thanks!

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Mar 20, 2024
@Strum355 Strum355 mentioned this pull request Mar 21, 2024
Merged
Strum355 added a commit to sourcegraph/sourcegraph-public-snapshot that referenced this pull request Mar 22, 2024
@Strum355
nix: bump to bazel 7.1 (#61326)
2f8990e 
Using my fork with open PR: NixOS/nixpkgs#295615

Im needing 7.1 sooner than later, given its new features and this repo also using 7.1 (so all new features are fair game for it!)

Cached version is pushed to sourcegraph-noah.cachix.org, which I've added at the top here so you too can avoid building it locally 🙂 

## Test plan

Tested locally 😎
@aaronmondal
Copy link
Contributor

7.1.1 has been released: https://github.com/bazelbuild/bazel/releases/tag/7.1.1

@Strum355
Copy link
Contributor Author

7.1.1 has been released: https://github.com/bazelbuild/bazel/releases/tag/7.1.1

7.1.1 changed the format of MODULE.bazel.lock in a way thatd require a lot more work in https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/tools/build-managers/bazel/bazel_7/bazel-repository-cache.nix, unless we want to take a hit in closure size by fetching everything instead of only a subset that this function does.
Although Im seeing this behaviour already with cpp-test-MODULE.bazel.lock (hence why I saw cpp tests fail locally I believe), so Im not sure what our next step should be

@DolceTriade
Copy link
Contributor

Do you think that we should try to get 7.1 merged first and then work on 7.1.1 after?

@olebedev olebedev removed their request for review April 6, 2024 01:22
@flurie
Copy link
Contributor

flurie commented Apr 9, 2024

Do you think that we should try to get 7.1 merged first and then work on 7.1.1 after?

I think this is best for now.

@aaronmondal
Copy link
Contributor

Could we merge this? There was a critical level CVE in rust https://blog.rust-lang.org/2024/04/09/cve-2024-24576.html and the rules_rust version that fixes it contains changes which require the reproducible feature from 7.1.0: bazelbuild/rules_rust#2453. I'm worried that the updated rules_rust work with the current Bazel 7.0.2.

malt3
malt3 approved these changes Apr 10, 2024
View reviewed changes
Copy link
Contributor

@malt3 malt3 left a comment

Choose a reason for hiding this comment

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

Tested this on some fairly large repositories on Linux x86_64

@Strum355
Copy link
Contributor Author

fwiw I don't have merge permissions for this. Just a lowly contributor

@cameron-martin
Copy link

@aaronmondal bazelbuild/rules_rust#2612 will fix rules_rust requiring Bazel 7.1

@wegank wegank added 12.approvals: 2 This PR was reviewed and approved by two reputable people and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Apr 12, 2024
@marsam marsam merged commit d97629a into NixOS:master Apr 25, 2024
25 checks passed
@aaronmondal aaronmondal mentioned this pull request May 6, 2024
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@malt3 malt3 malt3 approved these changes

@marsam marsam marsam approved these changes

@flurie flurie flurie approved these changes

@aherrmann aherrmann Awaiting requested review from aherrmann

@groodt groodt Awaiting requested review from groodt

@avdv avdv Awaiting requested review from avdv

@mboes mboes Awaiting requested review from mboes

@uri-canva uri-canva Awaiting requested review from uri-canva

@ylecornec ylecornec Awaiting requested review from ylecornec

@Profpatsch Profpatsch Awaiting requested review from Profpatsch

Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 12.approvals: 2 This PR was reviewed and approved by two reputable people
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@Strum355 @layus @avdv @aaronmondal @DolceTriade @flurie @cameron-martin @marsam @malt3 @wegank