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.26.1 -> 0.27.0 #63532

Merged
merged 4 commits into from
Jun 24, 2019
Merged

bazel: 0.26.1 -> 0.27.0 #63532

merged 4 commits into from
Jun 24, 2019

Conversation

guibou
Copy link
Contributor

@guibou guibou commented Jun 19, 2019

  • Upgrade to bazel 0.27
  • Fixs for newly introduced bin/bash hardcoded reference
  • Bazel now references remote_java_tools_xxx which contains prebuilt
    binaries. We prefetch them, fix them, and force bazel to use the
    fixed repository.

It also closes #63096

Motivation for this change

Bazel upgrade and bug fixing.

Note: instead of fixing #63096 and upgrading to bazel in two separates commits, I've done everything in once. I think it reduce the pull request pressure.

There is no test which proves that I really fixed stuffs, but I'm now able to build rules_haskell with bazel 0.27, which was not possible due to #63096. See tweag/rules_haskell#956

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.

@guibou
Copy link
Contributor Author

guibou commented Jun 19, 2019

Please, wait before merging. I'm having a regression in rules haskell and I'd like to understand if it comes from this change or not.

That's OK. I screwed stuff by a last minute refactoring, but that's OK now.

@FRidh
Copy link
Member

FRidh commented Jun 20, 2019

Duplicate of #63451.

@Profpatsch
Copy link
Member

Ah, can you cherry pick the first commit of #63451?

@guibou
Copy link
Contributor Author

guibou commented Jun 20, 2019

Ah, can you cherry pick the first commit of #63451?

I rather prefer fixing the test instead. I'll push that in a few minutes.

@guibou
Copy link
Contributor Author

guibou commented Jun 20, 2019

@Profpatsch I've pushed the repair of the installCheck and the python3 update. I'm currently running the bazel.passthru.tests I was unaware of.

Offline you suggested that I refactor the installCheck to move it to the baze.passthru.tests. I suggest to postpone this refactor, because I think that the important task is to get a working bazel back in nixpkgs.

But I totally agree that later we may move all the tests in bazel.passthru.tests, set it as an always run / fixed output derivation. This will enable network access and allows us to get ride of most hacks.

@Profpatsch
Copy link
Member

I suggest to postpone this refactor, because I think that the important task is to get a working bazel back in nixpkgs.

Yeah, but for that we need to turn the installCheck phase off completely. I just don’t want to have bazel builds that fail at the last possible moment anymore.

@Profpatsch
Copy link
Member

@GrahamcOfBorg build bazel.tests

@guibou
Copy link
Contributor Author

guibou commented Jun 21, 2019

Yeah, but for that we need to turn the installCheck phase off completely. I just don’t want to have bazel builds that fail at the last possible moment anymore.

I'm not sure I understand this sentence. Is there something I need to do to close this PR?

@guibou
Copy link
Contributor Author

guibou commented Jun 21, 2019

@GrahamcOfBorg build bazel.tests

I've rebased on master and fixed the deps sha256 to fix the darwin build failure.

@guibou
Copy link
Contributor Author

guibou commented Jun 21, 2019

@GrahamcOfBorg build bazel.tests

@guibou
Copy link
Contributor Author

guibou commented Jun 21, 2019

I amended some style only changes to match nixpkgs style guide, based on recommendations by @flokli.

- Fixs for newly introduced bin/bash hardcoded reference
- Bazel now references `remote_java_tools_xxx` which contains prebuilt
  binaries. We prefetch them, fix them, and force bazel to use the
  fixed repository.

It also closes NixOS#63096
All the dependencies of this phase are prefetched and provided to the
bazel environment using --override_repository.
@guibou
Copy link
Contributor Author

guibou commented Jun 22, 2019

@kalbasit

I suggest moving the bazelrc into $out/etc or $out/share. It feels a bit odd to have it at the top-level of $out.

Done. You are right. I used $out/etc, to match with default bazel which looks for the file in /etc.

Also, aren't you missing test and run?

The bazel docs says that test and run options "Inherits all options from build.", so I think we are good. I actually tested with bazel test and it works (but not tested with bazel run).

@flokli
Copy link
Contributor

flokli commented Jun 22, 2019

Do we run all tests? I see the hello_test function, but not sure if that's everything we could/should do…

@kalbasit
Copy link
Member

@guibou can you also include a Java test, similar to the existing Python test? Having a test will ensure Bazel will not be updated broken again.

@guibou
Copy link
Contributor Author

guibou commented Jun 24, 2019

@guibou can you also include a Java test, similar to the existing Python test? Having a test will ensure Bazel will not be updated broken again.

@kalbasit There is already a java test, it was in the disabled (but now re-enabled) checkPhase. It is simple, but it shows the ijar issue we previously had.

@guibou
Copy link
Contributor Author

guibou commented Jun 24, 2019

Do we run all tests? I see the hello_test function, but not sure if that's everything we could/should do…

We can do better, for sure.

The point of this PR was to fix the bazel currently included in nixpkgs. I did the upgrade to bazel 0.27 at the same time because I see no point duplicating effort here. There is a lot of other stuffs we can do to get a perfect bazel nix derivation, but I think that's not the priority now considering that the bazel which is currently provided by nixpkgs is broken.

@guibou
Copy link
Contributor Author

guibou commented Jun 24, 2019

@GrahamcOfBorg build bazel.tests

@flokli
Copy link
Contributor

flokli commented Jun 24, 2019

The derivation ran

//examples/cpp:hello-success_test                                        PASSED in 0.0s
//examples/java-native/src/test/java/com/example/myproject:hello         PASSED in 0.2s

I guess this is enough coverage to see at a quick glimpse if anything is utterly broken.

We can probably add more tests in a later PR, nothing that should block this bump, given it also fixes the bazel build currently being entirely broken.

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.

bazel: building java targets fails because bazel downloads ijar binary
5 participants