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 3 #87726

Merged
merged 2 commits into from May 29, 2020
Merged

Bazel 3 #87726

merged 2 commits into from May 29, 2020

Conversation

@avdv
Copy link
Contributor

avdv commented May 13, 2020

Motivation for this change

Bazel 3 has been released.

edit: I just realized that there already was #85356 which was closed eventually.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@avdv avdv requested a review from Profpatsch as a code owner May 13, 2020
@avdv avdv force-pushed the avdv:bazel_3 branch from ccb7c9d to fe8d938 May 13, 2020
@ofborg ofborg bot requested a review from mboes May 13, 2020
@Profpatsch
Copy link
Member

Profpatsch commented May 14, 2020

cc @uri-canva who wrote the first update PR.

@uri-canva
Copy link
Contributor

uri-canva commented May 14, 2020

Note you need to update all the hashes of the fetch derivations that use buildBazelPackage, as they depend on the bazel version.

@uri-canva
Copy link
Contributor

uri-canva commented May 14, 2020

That's probably why you're getting this failure:

error: build of '/nix/store/h4pv5mg2qif45075qc06iaqv5ib07r38-bazel-watcher-0.13.0.drv', '/nix/store/k63r46x8vz7nf58m009im1r1d4mmffa6-bazel-test-shebangs.drv' failed
@avdv
Copy link
Contributor Author

avdv commented May 14, 2020

Note you need to update all the hashes of the fetch derivations that use buildBazelPackage, as they depend on the bazel version.

Ok, thanks for that hint. I'll look into this...

@avdv avdv force-pushed the avdv:bazel_3 branch from fe8d938 to 96b1152 May 20, 2020
@avdv avdv requested review from FRidh and jonringer as code owners May 20, 2020
@avdv avdv force-pushed the avdv:bazel_3 branch from 96b1152 to b36e351 May 20, 2020
@avdv
Copy link
Contributor Author

avdv commented May 20, 2020

I have updated the hashes of all the packages using buildBazelPackage.

And I also added a patch to reverse commit bazelbuild/bazel@15d80dc because it made it hard to replace the python shebang line (the nix-build . -A bazel.tests run failed because of this).

BTW, is there an easy way to update the hashes, short of doing it manually?

@ofborg ofborg bot added the 6.topic: python label May 20, 2020
@ofborg ofborg bot requested review from kalbasit, timokau, andrew-d and uri-canva May 20, 2020
@kalbasit
Copy link
Member

kalbasit commented May 21, 2020

@avdv we could file a patch upstream to move the whole shebang to a java variable so we could substitute it out on our end, whst do you think? Might be difficult to keep maintaining this revert.

@avdv
Copy link
Contributor Author

avdv commented May 21, 2020

Yes, we could do that. But how likely is it that they except such a change?

The plan would be to apply that patch here until after it is merged in an upstream release, right?

@Profpatsch
Copy link
Member

Profpatsch commented May 24, 2020

Do we have any reverse dependencies in nixpkgs which depend on bazel 2? If not, I’d prefer we remove the bazel 2 files.

@avdv
Copy link
Contributor Author

avdv commented May 25, 2020

Do we have any reverse dependencies in nixpkgs which depend on bazel 2? If not, I’d prefer we remove the bazel 2 files.

👍 It seems every project that used bazel 2 before, can be build with bazel 3 now.

@avdv
Copy link
Contributor Author

avdv commented May 25, 2020

we could file a patch upstream to move the whole shebang to a java variable so we could substitute it out on our end, whst do you think? Might be difficult to keep maintaining this revert.

@kalbasit I tried to implement that idea:

--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java	2020-05-25 14:46:01.608403087 +0200
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java	2020-05-25 14:50:52.881398320 +0200
@@ -238,14 +238,15 @@
         // TODO(#8685): Remove this special-case handling as part of making the proper shebang a
         // property of the Python toolchain configuration.
         String pythonExecutableName = OS.getCurrent() == OS.OPENBSD ? "python3" : "python";
+        String pythonShebang = "#!/usr/bin/env " + pythonExecutableName;
         ruleContext.registerAction(
             new SpawnAction.Builder()
                 .addInput(zipFile)
                 .addOutput(executable)
                 .setShellCommand(
                     shExecutable,
-                    "echo '#!/usr/bin/env "
-                        + pythonExecutableName
+                    "echo '"
+                        + pythonShebang
                         + "' | cat - "
                         + zipFile.getExecPathString()
                         + " > "

and then replacing the pythonShebang variable with

substituteInPlace "$path" \
  --replace '+ pythonShebang' "+ \"${python3}/bin/python\"" \
  ...

What do you think?

@FRidh
Copy link
Member

FRidh commented May 25, 2020

What is the issue with the shebang? #!/usr/bin/env python3 seems fine for patch-shebangs.

@avdv
Copy link
Contributor Author

avdv commented May 25, 2020

What is the issue with the shebang? #!/usr/bin/env python3 seems fine for patch-shebangs.

@FRidh What do you mean? What is a patch-shebang?

The point is to replace the "/usr/bin/env python3" call with the path to the python binary in the resulting bazel derivation. The replacement currently fails because of upstream code changes, which later makes the build error out.

Do you have any better idea to resolve this?

@FRidh
Copy link
Member

FRidh commented May 25, 2020

patch-shebangs is a script we run (automatically) as part of our builds to exactly do this, convert such shebangs to absolute paths in the nix store.

@Profpatsch
Copy link
Member

Profpatsch commented May 25, 2020

@FRidh pretty sure there was a stupid reason we need this patch. Either because the file is used at build time or because it’s zipped into a jar at the end.

@avdv
Copy link
Contributor Author

avdv commented May 28, 2020

@avdv are you okay with the remaining changes or should I take a look?

@Profpatsch I just haven't had the time working on this PR again integrating all the remaining review comments.

I get to it this evening, probably.

@avdv avdv force-pushed the avdv:bazel_3 branch from b36e351 to 47e0b09 May 28, 2020
@avdv
Copy link
Contributor Author

avdv commented May 28, 2020

@Profpatsch I could spare a few minutes and updated the branch! 😃

(and rebased against master)

@avdv avdv force-pushed the avdv:bazel_3 branch 2 times, most recently from af81315 to 9d5c0ea May 28, 2020
@ofborg ofborg bot added the 8.has: clean-up label May 28, 2020
@ofborg ofborg bot requested a review from uri-canva May 28, 2020
@Profpatsch
Copy link
Member

Profpatsch commented May 28, 2020

It looks like the build is failing with a missing executable in a genrule phase:

ERROR: /build/bazel_src/src/BUILD:305:2: Executing genrule //src:embedded_tools_nojdk failed (Exit 2): bash failed: error executing command

https://logs.nix.ci/?key=nixos/nixpkgs.87726&attempt_id=25543299-77f2-4d2e-b4c1-901c0877dbc0

@avdv
Copy link
Contributor Author

avdv commented May 28, 2020

It looks like the build is failing with a missing executable in a genrule phase:

My interpretation is that the create_embedded_tools skript is somehow broken:

bazel-out/host/bin/src/create_embedded_tools: line 2: $'PK\003\004': command not found

It contains some sort of zip header where it would have expected a command. It also fails locally for me. How can I look at the output / disable the sandbox perhaps?

@Profpatsch
Copy link
Member

Profpatsch commented May 28, 2020

How can I look at the output / disable the sandbox perhaps?

nix-build --keep-failed

@avdv
Copy link
Contributor Author

avdv commented May 28, 2020

nix-build --keep-failed

Already did that, but this does not contain the /build files:

$ ls -l /tmp/nix-build-bazel-3.1.0.drv-0/bazel_src
...
lrwxrwxrwx  1 nixbld1 nixbld     43 May 28 16:15 bazel-bazel_src -> /build/bazel_CicdGTsu/out/execroot/io_bazel
lrwxrwxrwx  1 nixbld1 nixbld     64 May 28 16:15 bazel-bin -> /build/bazel_CicdGTsu/out/execroot/io_bazel/bazel-out/k8-opt/bin
lrwxrwxrwx  1 nixbld1 nixbld     53 May 28 16:15 bazel-out -> /build/bazel_CicdGTsu/out/execroot/io_bazel/bazel-out
lrwxrwxrwx  1 nixbld1 nixbld     69 May 28 16:15 bazel-testlogs -> /build/bazel_CicdGTsu/out/execroot/io_bazel/bazel-out/k8-opt/testlogs

the symlinks are broken.

@avdv
Copy link
Contributor Author

avdv commented May 28, 2020

Although I cannot really see why, I think the shebang patch broke this. 🔬

yes, of course 🤦 -- the shebang is missing afterwards...

OK, now it's failing again with:

Found files in the bazel distribution with illegal shebangs.
Replace those by explicit Nix store paths.
Python scripts should not use `bin/env python' but the Python interpreter's store path.
builder for '/nix/store/vjxchkqlpz84d35wll1js3d1l0c6qgi4-bazel-test-shebangs.drv' failed with exit code 1

Probably because the "bin/env python" string is still to be found inside class file although the variable is never used -- my idea was that the compiler would remove it completely.

@avdv avdv force-pushed the avdv:bazel_3 branch from 9d5c0ea to f7ffc33 May 28, 2020
@avdv
Copy link
Contributor Author

avdv commented May 28, 2020

So, yes the variable was still to be found inside the class file (might be debugging information). Locally, I made this change which made the tests pass again:

-          --replace '+ pythonShebang' "+ \"#!${python3}/bin/python\"" \
+          --replace '#!/usr/bin/env " + pythonExecutableName' "#!${python3}/bin/python\"" \

So instead of replacing the pythonShebang variable where it is used, I am replacing its value when it is initialized. Which kind of makes introducing the variable in the first place a bit useless... What do you think @kalbasit ?

My first attempt actually was to replace the original code spanning multiple lines, but that did not work since I tried to use globbing, probably.

(since the replacement is really only applicable in that one file, I'll probably introduce a single replaceInPlace call instead to avoid all these "WARNING: pattern '+ pythonShebang' doesn't match anything in file '" warnings)

avdv added 2 commits May 13, 2020
* drop bazel_2
* update hashes of fetch derivations that use `buildBazelPackage`
@avdv avdv force-pushed the avdv:bazel_3 branch from f7ffc33 to 694769e May 29, 2020
@avdv
Copy link
Contributor Author

avdv commented May 29, 2020

I updated the branch again, fixing the shebang-test.

The original branch using the reversed patch of the upstream change is still available here: https://github.com/avdv/nixpkgs/tree/bazel_3_a

@Profpatsch
Copy link
Member

Profpatsch commented May 29, 2020

Looks good, the linux build is passing now. We have a small test of python_binary, so I assume it should be fine now. If not, we can always add more tests.

@Profpatsch
Copy link
Member

Profpatsch commented May 29, 2020

All checks have passed, LGTM!

Bazel 3.2 was released two days ago 😝

@Profpatsch Profpatsch merged commit 1c5386f into NixOS:master May 29, 2020
16 checks passed
16 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
bazel, bazel.passthru.tests, bazel_3, bazel_3.passthru.tests on aarch64-linux Success
Details
bazel, bazel.passthru.tests, bazel_3, bazel_3.passthru.tests on x86_64-linux Success
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="694769e"; rev="694769e221714c1b88597cbaf1c69c1bea4a6c6d"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="694769e"; rev="694769e221714c1b88597cbaf1c69c1bea4a6c6d"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="694769e"; rev="694769e221714c1b88597cbaf1c69c1bea4a6c6d"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="694769e"; rev="694769e221714c1b88597cbaf1c69c1bea4a6c6d"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="694769e"; rev="694769e221714c1b88597cbaf1c69c1bea4a6c6d"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="694769e"; rev="694769e221714c1b88597cbaf1c69c1bea4a6c6d"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="694769e"; rev="694769e221714c1b88597cbaf1c69c1bea4a6c6d"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@avdv avdv mentioned this pull request May 29, 2020
4 of 10 tasks complete
@avdv
Copy link
Contributor Author

avdv commented May 29, 2020

Bazel 3.2 was released two days ago stuck_out_tongue_closed_eyes

New PR here: #89157 🚀

@avdv avdv deleted the avdv:bazel_3 branch May 29, 2020
@kalbasit
Copy link
Member

kalbasit commented May 29, 2020

So, yes the variable was still to be found inside the class file (might be debugging information). Locally, I made this change which made the tests pass again:

-          --replace '+ pythonShebang' "+ \"#!${python3}/bin/python\"" \

+          --replace '#!/usr/bin/env " + pythonExecutableName' "#!${python3}/bin/python\"" \

So instead of replacing the pythonShebang variable where it is used, I am replacing its value when it is initialized. Which kind of makes introducing the variable in the first place a bit useless... What do you think @kalbasit ?

actually replacing the variable definition is what I had in mind because it’s less error prone than the quoted echo part. BTW did you file a PR upstream?

Excited to jump on Bazel3, thanks for working on this!

@avdv
Copy link
Contributor Author

avdv commented May 29, 2020

actually replacing the variable definition is what I had in mind because it’s less error prone than the quoted echo part.

But it just looks so cumbersome... And by the specificity of the variable name I thought it was quite safe. Anyway, I am still learning and try to understand what's best practice when doing things.

BTW did you file a PR upstream?

No, not yet. Thanks for reminding me! 😁

bazel-io pushed a commit to bazelbuild/bazel that referenced this pull request Jun 17, 2020
Building on Nix / NixOS we need to avoid using `/usr/bin/env` to determine the python interpreter (or other executables).

This change makes it easier for us to replace the shebang line when building the derivation of bazel with a fixed path to a python3 executable.

See NixOS/nixpkgs#87726 for details.

Closes #11535.

PiperOrigin-RevId: 316886082
@avdv
Copy link
Contributor Author

avdv commented Jun 18, 2020

@kalbasit the PR on bazel has been merged: bazelbuild/bazel@dfaf06a

On the next version bump, we should be able to drop the patch.

Oh, speaking of which: bazel 3.3.0 has been released a few hours ago... (I cannot easily see whether it has the patch or not -- edit: no it is not yet inside the release, late by 2h)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.