bazel: remove dependence on buildFHSUserEnv #23074

izuk · 2017-02-22T12:57:59Z

Motivation for this change

Reduce the complexity of default.nix for bazel.

The main issues I found were:

Dependence on /bin/bash everywhere.
Symlink recursion at build time.

For the former I manually patched a bunch of the shell scripts and the main Java command helper. The patch could probably be pruned down to a handful of places if we don't care about the tests.

For the latter I set TMPDIR=/tmp before compiling. This is ugly and I welcome a better solution.

I'm also not clear on how necessary propagatedBuildInputs is, or whether this works on Darwin. I would appreciate some help/advice there.

Things done

Tested using sandboxing
(nix.useSandbox on NixOS,
or option build-use-sandbox in nix.conf
on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- Linux
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

mention-bot · 2017-02-22T12:58:00Z

@izuk, thanks for your PR! By analyzing the history of the files in this pull request, we identified @IreneKnapp, @benley and @teh to be potential reviewers.

globin · 2017-02-22T13:48:24Z

patchShebangs . should be used instead of a manual patch

izuk · 2017-02-22T14:52:45Z

I switched to using patchShebangs, except for a few cases.

Mic92 · 2017-02-22T15:05:16Z

pkgs/development/tools/build-managers/bazel/default.nix

-in bazelBinary
+  sourceRoot = ".";
+
+  patches = [ ./bin_to_env.patch ];


The patch is quite big. Would it make sense to manage this patch external in a git like we do for ruby to make rebasing easier?

I wonder if the Bazel team would accept most of these changes upstream. Using /usr/bin/env bash seems like a pretty universally sane thing to do in place of /bin/bash, unless you actively distrust PATH in the environment, and bazel manages PATH itself when it sets up build sandboxes.

I can ask the devs if they'd be OK with that. Maybe next week :)

Also, the patch is smaller now that I use patchShebangs.

@benley I asked on the conda (a mostly Python package manager) issue tracker once if they could change to /usr/bin/env bash and got back

I've never come across a Unix system, which does not have /bin/bash installed.
...
The easiest fix would be to create a link in /bin/bash to the location where bash is located on that system, I would say

@FRidh then those people have never used something else but linux and MacOS X. On FreeBSD bash is installed in /usr/local/bin or in Solaris it is even in /opt.

IreneKnapp · 2017-02-22T18:35:40Z

I'm not the maintainer nor do I have any formal connection to NixOS, but please verify manually that the built bazel can build C++ and Java binaries which, themselves, work properly.

Ideally someday there would be an automated test for this.

izuk · 2017-02-23T04:54:58Z

I've added a checkPhase that uses the built bazel to build and run two tests, for CPP and for Java.

I've also verified that this behaves as expected:

$ nix-env -f . -iA bazel
$ nix-shell -p gcc
$ bazel test examples/cpp/...

IreneKnapp · 2017-02-23T05:05:10Z

Awesome! Thank you for making this work.

izuk · 2017-02-23T18:05:22Z

Anything else I need to do for this to get approved?

globin

Otherwise generally looks good, although I don't like having large patches like this and should generally also be tried to fix it upstream.

globin · 2017-02-23T18:18:01Z

pkgs/development/tools/build-managers/bazel/bin_to_env.patch

+
+     try {
+-      new Command(new String[] {"/bin/true", null}).execute();
+      new Command(new String[] {"/usr/bin/env", "true", null}).execute();


true is a shell built-in normally and won't work through /usr/bin/env

true is also an executable provided by coreutils. That's essentially what the test (which now passes, BTW) uses.

Do you still want me to do something about this? If you object to this part of the patch I can just remove it. It doesn't affect the build, just the unit tests.

imo this ought to stay, since it looks like it won't work without the patch. In theory you could patch in the /nix/store path to bin/true but if it's findable in $PATH then this is probably good enough

Again, do I need to do something about this? I'd like to move on, and this pull request is holding me back.

let's merge it. It sounds like the change works and it's an improvement over the FHSUserEnv machinery.

globin · 2017-02-27T23:42:07Z

The test phase is failing when in sandboxed mode due to no /usr/bin/env being available there.
https://hydra.nixos.org/build/49443813

//examples/java-native/src/test/java/com/example/myproject:hello         FAILED in 1 out of 2 in 0.1s
  /tmp/nix-build-bazel-0.4.4.drv-0/_bazel_nixbld/c8355d9ca7205113d128945ce10aaae4/execroot/nix-build-bazel-0.4.4.drv-0/bazel-out/local-fastbuild/testlogs/examples/java-native/src/test/java/com/example/myproject/hello/test.log

Executed 1 out of 1 test: 1 fails locally.
note: keeping build directory ‘/tmp/nix-build-bazel-0.4.4.drv-0’
builder for ‘/nix/store/9i9ibabnsb56wds894hzp0b6a7d3ka86-bazel-0.4.4.drv’ failed with exit code 3
error: build of ‘/nix/store/9i9ibabnsb56wds894hzp0b6a7d3ka86-bazel-0.4.4.drv’ failed

$ cat /tmp/nix-build-bazel-0.4.4.drv-0/_bazel_nixbld/c8355d9ca7205113d128945ce10aaae4/execroot/nix-build-bazel-0.4.4.drv-0/bazel-out/local-fastbuild/testlogs/examples/java-native/src/test/java/com/example/myproject/hello/test.log
exec ${PAGER:-/usr/bin/less} "$0" || exit 1
-----------------------------------------------------------------------------
external/bazel_tools/tools/test/test-setup.sh: /tmp/nix-build-bazel-0.4.4.drv-0/_bazel_nixbld/c8355d9ca7205113d128945ce10aaae4/bazel-sandbox/343e6c44-d0f3-45dd-a9c6-35372cdbe3ef-3/execroot/nix-build-bazel-0.4.4.drv-0/bazel-out/local-fastbuild/bin/examples/java-native/src/test/java/com/example/myproject/hello.runfiles/io_bazel/examples/java-native/src/test/java/com/example/myproject/hello: /usr/bin/env: bad interpreter: No such file or directory

benley · 2017-02-28T00:02:25Z

Weird. Seems like an easy fix at least: add coreutils to buildInputs. I'm testing that out now.

izuk · 2017-02-28T00:07:40Z

It might be nicer to change the patch to replace /bin/bash with @bash@/bin/bash and then run one of the substitute* scripts in postPatch. I think that would also require propagatedBuildInputs. Not sure.

I can look at that later this week if anyone thinks that's a good idea.

benley · 2017-02-28T00:48:30Z

It seems like patching the shebang lines may be necessary after all; even with coreutils in propagatedBuildInputs there seems to be nothing at /usr/bin/env during checkPhase.

izuk force-pushed the bazel branch from 15c7f4c to f8cd498 Compare February 22, 2017 14:50

Mic92 reviewed Feb 22, 2017

View reviewed changes

bazel: remove dependence on buildFHSUserEnv

59bb3d1

izuk force-pushed the bazel branch from f8cd498 to 59bb3d1 Compare February 23, 2017 04:43

globin requested changes Feb 23, 2017

View reviewed changes

benley merged commit 0c3afbd into NixOS:master Feb 26, 2017

benley mentioned this pull request Feb 28, 2017

bazel: Log test errors to stderr during checkPhase #23255

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bazel: remove dependence on buildFHSUserEnv #23074

bazel: remove dependence on buildFHSUserEnv #23074

izuk commented Feb 22, 2017

mention-bot commented Feb 22, 2017

globin commented Feb 22, 2017

izuk commented Feb 22, 2017

Mic92 Feb 22, 2017

benley Feb 23, 2017 •

edited

izuk Feb 23, 2017

izuk Feb 23, 2017

FRidh Feb 23, 2017

Mic92 Feb 27, 2017 •

edited

IreneKnapp commented Feb 22, 2017

izuk commented Feb 23, 2017

IreneKnapp commented Feb 23, 2017

izuk commented Feb 23, 2017

globin left a comment

globin Feb 23, 2017

izuk Feb 23, 2017

izuk Feb 25, 2017

benley Feb 25, 2017

izuk Feb 26, 2017

benley Feb 26, 2017

globin commented Feb 27, 2017 •

edited

benley commented Feb 28, 2017

izuk commented Feb 28, 2017 •

edited

benley commented Feb 28, 2017

bazel: remove dependence on buildFHSUserEnv #23074

bazel: remove dependence on buildFHSUserEnv #23074

Conversation

izuk commented Feb 22, 2017

Motivation for this change

Things done

mention-bot commented Feb 22, 2017

globin commented Feb 22, 2017

izuk commented Feb 22, 2017

Choose a reason for hiding this comment

benley Feb 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mic92 Feb 27, 2017 • edited

Choose a reason for hiding this comment

IreneKnapp commented Feb 22, 2017

izuk commented Feb 23, 2017

IreneKnapp commented Feb 23, 2017

izuk commented Feb 23, 2017

globin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

globin commented Feb 27, 2017 • edited

benley commented Feb 28, 2017

izuk commented Feb 28, 2017 • edited

benley commented Feb 28, 2017

benley Feb 23, 2017 •

edited

Mic92 Feb 27, 2017 •

edited

globin commented Feb 27, 2017 •

edited

izuk commented Feb 28, 2017 •

edited