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_6: init with a WIP implem #186106

Merged
merged 5 commits into from
Sep 2, 2022
Merged

bazel_6: init with a WIP implem #186106

merged 5 commits into from
Sep 2, 2022

Conversation

layus
Copy link
Member

@layus layus commented Aug 11, 2022

Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 22.11 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@layus
Copy link
Member Author

layus commented Aug 11, 2022

/cc @avdv @ylecornec @the-mikedavis @uri-canva Starting here the work to get bazel_6 packaged.

This does not work yet, I have lots of trouble with "ijar" internal tool that refuses to run. Any idea ?

@layus
Copy link
Member Author

layus commented Aug 12, 2022

@r2r-dev is this of any interest for you ATM ? I would love to do some peer programming

@uri-canva
Copy link
Contributor

It works for me on x86_64-darwin with this change:

diff --git a/pkgs/top-level/all-packages.nix b/pkgs/top-level/all-packages.nix
index 9431ba72bb0..1fe320bbb95 100644
--- a/pkgs/top-level/all-packages.nix
+++ b/pkgs/top-level/all-packages.nix
@@ -15459,12 +15459,14 @@ with pkgs;
     bazel_self = bazel_5;
   };

-  bazel_6 = callPackage ../development/tools/build-managers/bazel/bazel_6 {
+  bazel_6 = darwin.apple_sdk_11_0.callPackage ../development/tools/build-managers/bazel/bazel_6 {
     inherit (darwin) cctools;
-    inherit (darwin.apple_sdk.frameworks) CoreFoundation CoreServices Foundation;
+    inherit (darwin.apple_sdk_11_0.frameworks) CoreFoundation CoreServices Foundation;
     buildJdk = jdk11_headless;
     runJdk = jdk11_headless;
-    stdenv = if stdenv.cc.isClang then llvmPackages.stdenv else stdenv;
+    stdenv = if stdenv.isDarwin then
+      darwin.apple_sdk_11_0.stdenv else
+      if stdenv.cc.isClang then llvmPackages.stdenv else stdenv;
     bazel_self = bazel_6;
   };

@layus
Copy link
Member Author

layus commented Aug 16, 2022

Thanks a lot @uri-canva. To be honest I am puzzled as to why your patch works at all. I have pushed a fix that makes sense (to me) for my linux build. Would you mind trying it without your patch ? And if it fails, see if your patch still works ? That would help me a lot.

Thanks !

@uri-canva
Copy link
Contributor

I still need my patch to build at all, but even with the patch I'm now hitting issues like this:

bazel-out/darwin-fastbuild/bin/external/com_google_protobuf/_javac/duration_proto/libduration_proto-speed_tmp/com/google/protobuf/Duration.java:58: error: [strict] Using type com.google.protobuf.GeneratedMessageV3 from an indirect dependency (TOOL_INFO: "@com_google_protobuf//java/core"). See command below **
    com.google.protobuf.GeneratedMessageV3 implements
                       ^
bazel-out/darwin-fastbuild/bin/external/com_google_protobuf/_javac/duration_proto/libduration_proto-speed_tmp/com/google/protobuf/Duration.java:82: error: [strict] Using type com.google.protobuf.CodedInputStream from an indirect dependency (TOOL_INFO: "@com_google_protobuf//java/core:lite_runtime_only"). See command below **
      com.google.protobuf.CodedInputStream input,
                         ^
 ** Please add the following dependencies:
  @com_google_protobuf//java/core @com_google_protobuf//java/core:lite_runtime_only to @com_google_protobuf//:duration_proto
 ** You can use the following buildozer command:
buildozer 'add deps @com_google_protobuf//java/core @com_google_protobuf//java/core:lite_runtime_only' @com_google_protobuf//:duration_proto

I tried to add build --experimental_strict_java_deps=off to the .bazelrc but it wasn't enough, I probably need to add it to compile.sh too.

@uri-canva
Copy link
Contributor

Sorry was going to explain the changes then I realised they're quite big and very darwin specific so I figured I would just push them, let me know if you have any questions.

@uri-canva
Copy link
Contributor

If you're happy with the changes mark the PR as ready to review. They look good to me.

@ofborg ofborg bot requested a review from uri-canva August 23, 2022 09:33
Copy link
Member

@ErinvanderVeen ErinvanderVeen left a comment

Choose a reason for hiding this comment

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

Builds and tested with the bazel examples on NixOS.

@uri-canva
Copy link
Contributor

ping @layus

@uri-canva
Copy link
Contributor

@layus I'll go ahead and push this PR forward since I'm interested in having it merged, hope you don't mind. If you have changes you'd like to make we can fix them up in a follow up PR.

@uri-canva uri-canva marked this pull request as ready for review August 29, 2022 23:55
@uri-canva uri-canva mentioned this pull request Aug 30, 2022
13 tasks
@bobby285271 bobby285271 added 12.approvals: 2 This PR was reviewed and approved by two reputable people 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package labels Aug 30, 2022
@layus
Copy link
Member Author

layus commented Aug 30, 2022

@uri-canva Thanks for pushing this forward. I have no particular expectations on this PR. I already deployed the working linux version where I needed it. This is my way to share upstream.

What bugs me is that you had to patch Bazel around the "error: [strict] Using type [...] from an indirect dependency" java issue. On my side I discovered that commit 1f11d01 somehow fixed the bug. I wonder what part of these changes did it. Would you mind opening an issue upstream for these new patches ?

Another annoying part is that stdenv entries become darwin ones. Reading the code, it took me quite some time to understand that these darwin things are just stdenv on linux. I guess it's a nixpkgs-wide issue, but that delayed my feedback quite a lot (until I found some darwin users to discuss with).

But none of that should be a blocker for merging an unstable bazel_6 package. I have asked colleagues for a review. I will merge this once we get a proper review.

@layus
Copy link
Member Author

layus commented Aug 30, 2022

@uri-canva The reason why Erin's review is grey is because he does not have commit access. It's green otherwise. I do not like this feature as it seems to imply a difference in the review itself, when it only really about permissions. I see the use case for a green review when that reviewer could potentially merge the pr, but still...

@uri-canva
Copy link
Contributor

Yeah the darwin stdenv is quite confusing, hopefully we can remove it soon. You're right, the java error is due to something in the jdk, you can see some discussion in bazelbuild/bazel#16109. It's already fixed upstream, but the fix hasn't made it through all the different ways you can configure the jdk.

Copy link
Contributor

@uri-canva uri-canva left a comment

Choose a reason for hiding this comment

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

I just noticed something, was src-deps.json updated? update-srcDeps.py doesn't work with bazel 6 as it is now.

@uri-canva uri-canva self-requested a review August 31, 2022 12:35
@uri-canva
Copy link
Contributor

Ignore me, was using it wrong.

@@ -2,7 +2,8 @@
bazel
, bazelTest
, bazel-examples
, gccStdenv
, stdenv
, darwin
Copy link
Member

Choose a reason for hiding this comment

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

I agree with @layus's comment that this is confusing. If the strange double meaning (darwin = stdenv on Linux) can't be avoided, then a comment explaining it may be helpful.

Copy link
Member Author

Choose a reason for hiding this comment

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

As this is in the tests, and not in the main bazel_6 derivation, I think we can ignore this for now. Unless it changes the test significantly ?

@bobby285271 bobby285271 added 12.approvals: 1 This PR was reviewed and approved by one reputable person and removed 12.approvals: 2 This PR was reviewed and approved by two reputable people labels Aug 31, 2022
@uri-canva
Copy link
Contributor

Got some more information upstream, I think the strict deps thing is due to our java setup: bazelbuild/bazel#16146. I haven't been able to narrow it down any further than that, I'm ok with merging this as is and continuing looking into it later.

@layus
Copy link
Member Author

layus commented Sep 2, 2022

@uri-canva I made one cleanup commit. Let's wait CI and merge it.

@layus
Copy link
Member Author

layus commented Sep 2, 2022

@uri-canva For some reason, GH still thinks that you requested changes...

@ofborg ofborg bot requested a review from aherrmann September 2, 2022 11:03
@layus
Copy link
Member Author

layus commented Sep 2, 2022

@uri-canva
Copy link
Contributor

That's very strange, let me try to build it locally.

@uri-canva
Copy link
Contributor

Oh you only did a cleanup commit after the last one, I don't think that would have caused the issue, it might be some ofborg specific issue. Since it's a new derivation, let's try to merge it as is and see if it builds on hydra.

@uri-canva uri-canva merged commit a6e347f into NixOS:master Sep 2, 2022
@uri-canva
Copy link
Contributor

Whoops I just realised we should have updated the name of the PR.

@layus
Copy link
Member Author

layus commented Sep 2, 2022

Done 🎉 . Thanks a lot for your efforts !

@layus layus deleted the bazel-update branch September 2, 2022 19:13
@uri-canva
Copy link
Contributor

Darwin built on hydra, so the failure was an ofborg specific issue: https://hydra.nixos.org/eval/1779113?filter=bazel_6&compare=1779052&full=#tabs-aborted

However linux seems to have failed, though I don't understand the failure. Is it because of sandboxing?

@layus
Copy link
Member Author

layus commented Sep 5, 2022 via email

@uri-canva
Copy link
Contributor

Nice, we got a green build: https://hydra.nixos.org/build/190329458.

@boltzmannrain boltzmannrain mentioned this pull request Nov 14, 2024
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants