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

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

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

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...

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?

@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.

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?

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

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.

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?

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

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?

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.

@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 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.

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

(and rebased against master)

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

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?

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

nix-build --keep-failed

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.

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.

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)

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

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.

All checks have passed, LGTM!

Bazel 3.2 was released two days ago 😝

Bazel 3.2 was released two days ago stuck_out_tongue_closed_eyes

New PR here: #89157 🚀

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!

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! 😁

@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)