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: 0.24.0 -> 0.26.0 #62336

Merged
merged 5 commits into from
Jun 12, 2019
Merged

bazel: 0.24.0 -> 0.26.0 #62336

merged 5 commits into from
Jun 12, 2019

Conversation

groodt
Copy link
Contributor

@groodt groodt commented Jun 1, 2019

Motivation for this change

A few new versions of Bazel have been released. This bumps to 0.26.1 to stay recent.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@matthewbauer
Copy link
Member

@GrahamcOfBorg build bazel

@groodt
Copy link
Contributor Author

groodt commented Jun 1, 2019

My local Darwin build error is:

In file included from src/main/cpp/blaze_util_darwin.cc:24:
/nix/store/mffyf74zc4h2wic5lwx0kyx7gvwif0c5-Libsystem-osx-10.12.6/include/pthread/spawn.h:65:34: error: unknown type name 'qos_class_t'; did you mean 'au_class_t'?
                                 qos_class_t __qos_class);
                                 ^
/nix/store/mffyf74zc4h2wic5lwx0kyx7gvwif0c5-Libsystem-osx-10.12.6/include/bsm/audit.h:176:19: note: 'au_class_t' declared here
typedef u_int32_t       au_class_t;
                        ^
In file included from src/main/cpp/blaze_util_darwin.cc:24:
/nix/store/mffyf74zc4h2wic5lwx0kyx7gvwif0c5-Libsystem-osx-10.12.6/include/pthread/spawn.h:88:34: error: unknown type name 'qos_class_t'; did you mean 'au_class_t'?
                                 qos_class_t * __restrict __qos_class);
                                 ^
/nix/store/mffyf74zc4h2wic5lwx0kyx7gvwif0c5-Libsystem-osx-10.12.6/include/bsm/audit.h:176:19: note: 'au_class_t' declared here
typedef u_int32_t       au_class_t;
                        ^
2 errors generated.

@groodt
Copy link
Contributor Author

groodt commented Jun 2, 2019

My local linux build succeeds, but tests are failing with:

src/main/tools/process-wrapper-legacy.cc:58: "execvp(external/remote_java_tools_linux/java_tools/ijar/ijar, ...)": No such file or directory

@groodt groodt marked this pull request as ready for review June 2, 2019 01:41
@groodt groodt requested a review from Profpatsch as a code owner June 2, 2019 01:41
@uri-canva
Copy link
Contributor

uri-canva commented Jun 2, 2019

Here's a fix for the Darwin issue:

diff --git a/pkgs/development/tools/build-managers/bazel/default.nix b/pkgs/development/tools/build-managers/bazel/default.nix
index 3b23ca395e8..3a880541228 100644
--- a/pkgs/development/tools/build-managers/bazel/default.nix
+++ b/pkgs/development/tools/build-managers/bazel/default.nix
@@ -160,6 +160,9 @@ stdenv.mkDerivation rec {
         src/tools/xcode/realpath/BUILD \
         src/tools/xcode/stdredirect/BUILD \
         tools/osx/BUILD
+
+      # nixpkgs's libSystem cannot user pthread headers directly, must import GCD headers instead
+      sed -i -e "/#include <pthread\/spawn.h>/i #include <dispatch/dispatch.h>" src/main/cpp/blaze_util_darwin.cc

       # clang installed from Xcode has a compatibility wrapper that forwards
       # invocations of gcc to clang, but vanilla clang doesn't

@groodt
Copy link
Contributor Author

groodt commented Jun 2, 2019

Legend! Thanks @uri-canva I'll kick off a local test now. I think I'll have to fetch the java_tools_javac10_darwin-v3.1.zip as well.

I think I understand what is happening during the installCheckPhase. I've got it working if I build on Linux without Nix. Bazel downloads java_tools_javac10_linux-v3.1.zip when building the interface jars for junit4 used in the tests.

This is failing under Nix. I'm not 100% clear on how or if this should be fixed. I wonder if the whole approach to these tests may fail, given that Bazel can fetch rules remotely.

@Lassulus
Copy link
Member

Lassulus commented Jun 2, 2019

@GrahamcOfBorg build bazel

@groodt
Copy link
Contributor Author

groodt commented Jun 2, 2019

Success on Darwin, failure on Linux. Must be a first! 😂

I'm looking into the Linux errors. I didn't have sandbox enabled locally, so I'll do that for Linux which will hopefully help track down errors.

@matthewbauer
Copy link
Member

@uri-canva Thanks! Opened this issue to track what's going on: #62555

@groodt
Copy link
Contributor Author

groodt commented Jun 3, 2019

@matthewbauer Any chance you can kick off another build. Linux only. I'm unable to run sandboxed builds in a Docker container at the moment. I've fixed the previous error with rules_sass in f1a511a

@uri-canva
Copy link
Contributor

uri-canva commented Jun 3, 2019

I suspect the Bazel tests fail on Linux because of the network sandboxing. The way the building of the tests should be done to account for that is the same way buildBazelPackage does it: first run bazel fetch as a fixed output derivation with networking enabled, then run bazel test with networking disabled.

We could rewrite the whole derivation to split it in two: first build the bootstrap bazel, then use boostrap bazel with buildBazelPackage to build and test Bazel. That also mirrors what compile.sh does.

It's a lot of work though, we might want to skip the tests on Linux in the meantime.

@groodt
Copy link
Contributor Author

groodt commented Jun 3, 2019

@Profpatsch This is now ready for review. Please kick off a build to confirm.

I expect it to pass on Darwin. Fail on Linux. I'm able to reproduce the Linux failure on my local VM.

@groodt
Copy link
Contributor Author

groodt commented Jun 3, 2019

We could rewrite the whole derivation to split it in two: first build the bootstrap bazel, then use boostrap bazel with buildBazelPackage to build and test Bazel. That also mirrors what compile.sh does.

Yes, I think given the way Bazel works, this is the only sensible solution. I think I understand what is necessary. Will that second inside buildBazelPackage be cached in the Nix store?

@uri-canva
Copy link
Contributor

Yes, the deps derivation is separate from the build derivation and they get cached separately.

@Profpatsch
Copy link
Member

So we’re skipping 0.25 alltogether? #61097

@groodt
Copy link
Contributor Author

groodt commented Jun 3, 2019

So we’re skipping 0.25 alltogether? #61097

The Bazel release cycle is too frequent to do 1:1 Nix releases. I certainly don't have time to do it. The functionality I want is in 0.26. In a week or 2 there will 0.27....

@Profpatsch
Copy link
Member

@GrahamcOfBorg build bazel.tests

@Profpatsch
Copy link
Member

The linux test fails.

@groodt
Copy link
Contributor Author

groodt commented Jun 4, 2019

The linux test fails.

@Profpatsch Yes, thanks, it is expected. We just wanted to confirm if it fails on CI with the same error as we get locally. It does, so that's great, we can hopefully fix it.

The failure is due to the sandbox blocking network access during the tests. Bazel isn't very good to test in this way, as rules can go and download other rules at runtime. What are your thoughts to fixing it?

@Profpatsch
Copy link
Member

What are your thoughts to fixing it?

By now I’m tempted to disable the checkPhase (doCheck = false;) and rely on the manual tests we run afterwards. We can’t keep up with their (shitty) WORKSPACE stuff, they don’t really care about other build systems.

@Profpatsch
Copy link
Member

The problem now is that the fetch arbitrary binaries (external/remote_java_tools_linux/java_tools/ijar/ijar) and try to execute them, we must patch those first or add their dependencies to the test environment.

@uri-canva
Copy link
Contributor

The java tools currently live here: https://github.com/bazelbuild/bazel/tree/master/src/java_tools, but they'll move them to https://github.com/bazelbuild/java_tools eventually. We can build them from source in the nix derivation using the bootstrap bazel that should be possible to build without network access.

@uri-canva
Copy link
Contributor

Sorry I'm just commenting and not helping much on the PR this time, I don't have enough time to dedicate to it at the moment.

@uri-canva
Copy link
Contributor

We can also patch out some of the remote files as long as we delete the references to them: I cannot believe the rules_sass and rules_nodejs are needed for building Bazel itself, they're probably used to render the documentation.

@groodt
Copy link
Contributor Author

groodt commented Jun 5, 2019

@Profpatsch

By now I’m tempted to disable the checkPhase (doCheck = false;) and rely on the manual tests we run afterwards.

I agree that this is the best course of action for now. PR updated to reflect this. Passing locally on my Linux VM.

I'll try to find some time to submit an additional PR to change the approach of the Bazel derivation as @uri-canva is suggesting.

@kalbasit kalbasit mentioned this pull request Jun 6, 2019
10 tasks
@groodt
Copy link
Contributor Author

groodt commented Jun 12, 2019

Great! Seems to have passed. I'll try a quick bump to 0.26.1 sometime today.

@groodt
Copy link
Contributor Author

groodt commented Jun 12, 2019

The build works locally for me on Darwin (no sandbox) and a NixOS VM (sandbox = true).

I'll update the title, description and squash the commits.

@groodt groodt changed the title bazel: 0.24.0 -> 0.26.0 bazel: 0.24.0 -> 0.26.1 Jun 12, 2019
@Profpatsch
Copy link
Member

Is it okay if we leave the commits as-is? I’d only squash in the fixup commits (git rebase --autosquash).

@groodt
Copy link
Contributor Author

groodt commented Jun 12, 2019

Is it okay if we leave the commits as-is? I’d only squash in the fixup commits (git rebase --autosquash).

Apologies, I think I'm too late and I've squashed it all down. In some of my other nixpkgs contributions, the reviewer was particular that I squashed everything down into a single commit with a particular commit message. Maybe that's not standard across the whole repo or is more nuanced and can be more than a single commit.

@Profpatsch
Copy link
Member

Normally multiple commits for separate changes are not a problem. I’ll force-push the split commits.

groodt and others added 5 commits June 12, 2019 14:09
`py_test` tries to download unnecessary dependencies at runtime. We
can just as well run it to check the assertion.

Upstream issue: bazelbuild/bazel#8575
Factor out the common parts of tests & cache the bazel
self-extraction (ugh) to a common store path.
xe is such a trivial package, it should build on every platform that
supports a CC compiler.
@Profpatsch
Copy link
Member

Okay, I rebased to master & autosquashed the fixup commits. Will merge once it’s green again.

@Profpatsch
Copy link
Member

Profpatsch commented Jun 12, 2019

Ah, I’m sorry, I didn’t see the bump to 0.26.1. Meh, can you push the diff as a separate PR?

@Profpatsch Profpatsch merged commit cc62736 into NixOS:master Jun 12, 2019
@Profpatsch Profpatsch changed the title bazel: 0.24.0 -> 0.26.1 bazel: 0.24.0 -> 0.26.0 Jun 12, 2019
@kalbasit
Copy link
Member

kalbasit commented Jun 12, 2019

I'm putting a quick PR together for 0.26.1.

EDIT: #63038

@groodt
Copy link
Contributor Author

groodt commented Jun 12, 2019

Thanks @Profpatsch @kalbasit

@timokau
Copy link
Member

timokau commented Jun 20, 2019

I just noticed that this changed the hash of the fetch phase of dm-sonnet. We probably have to be more careful with bazel updates in the future if we're using it in fixed output derivations.

@Profpatsch
Copy link
Member

I just noticed that this changed the hash of the fetch phase of dm-sonnet. We probably have to be more careful with bazel updates in the future if we're using it in fixed output derivations.

Uuuuuuuuuuuuuuugh, I suggest we just don’t.

@timokau But for the while being, can you add a tests.downstreamPackages = { inherit <list of packages that use fetchbazel>; }?

@timokau
Copy link
Member

timokau commented Jun 21, 2019

Don't do what? Don't be careful seems like a bad idea to me ;) And we're already using it in fixed output derivations, I don't see how we could avoid that when building bazel packages.

Would that actually help? Since they're fixed output, they will just be fetched from the binary cache and therefore pass the tests even though they're broken.

@kalbasit
Copy link
Member

@timokau you can probably override one of the phases of the fetcher to invalidate it's input, and force it to compile. Something like echo ${version} should do.

@timokau
Copy link
Member

timokau commented Jun 22, 2019

Interesting idea! I feel like there is a bit of a "just because you can doesn't mean that you should" going on here, but here we go: #63645

I have created a separate attribute for the derivation instead of a list, because a list wouldn't get built with nix-build -A bazel.test.

@Profpatsch
Copy link
Member

Don't do what? Don't be careful seems like a bad idea to me ;) And we're already using it in fixed output derivations, I don't see how we could avoid that when building bazel packages.

Don’t ever use fixed output derivations.

@timokau
Copy link
Member

timokau commented Jun 22, 2019

How else should we fetch bazel dependencies?

I'd expect the fetch output to become deterministic between releases once bazel is stable. If it still isn't, we can look into how they differ between releases and clean up the fetch result accordingly (similar to what we do with fetchpatch).

@Profpatsch
Copy link
Member

How else should we fetch bazel dependencies?

We do it like with all other (well-behaved) package managers and generate nix expressions for WORKSPACE dependencies. I already wrote a mockup hack for updating bazel proper: https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/tools/build-managers/bazel/update-srcDeps.py
But really, bazel devs should pull the stick out of their asses, provide a reasonable interface and become a nice player in the open source world.

@uri-canva
Copy link
Contributor

Yeah Bazel is really really biased towards monorepos with full vendoring.

I think something good integration-wise can be done with resolved workspaces and provided external repositories, but both mechanisms are very much in their infancy and it's unclear how well supported they are:

https://blog.bazel.build/2018/07/09/bazel-sync-and-resolved-file.html
bazelbuild/bazel#5252

@Profpatsch
Copy link
Member

Ah, yes, that’s the better way tbh. It’s a skylark file, so we should be able to cheat our way by evaling it in a python interpreter.

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.

7 participants