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

Jump to bottom

bazel: fix python stub paths #53988

Merged
merged 1 commit into from Jan 15, 2019
Merged

bazel: fix python stub paths #53988

merged 1 commit into from Jan 15, 2019

Conversation

picnoir
Copy link
Member

@picnoir picnoir commented Jan 15, 2019 •
edited

Motivation for this change

Since the 0.21 upgrade, the host $PATH is not forwarded anymore by default to the sandboxes in charge to realize Bazel actions. This default change broke the py_binary rule among other things.

Every python binary is wrapped in a stub in charge to setup the execution environment. Currently, this stub's shebang points to a /usr/bin/env python which cannot be resolved with the current $PATH. This results in breaking any build pipeline requiring the use of python at some point. On top of the incorrect shebang, the stub template is unable to find the actual python binary using SearchPath.

This PR fixes those two things by re-writing the stub template shebang to the actual python binary and by substituting the faulty default python binary lookup to the right one.

Notes:

  • The patch file instead of a substitute-in-place is intentional: we probably want to build to fail if this part of the template logic changed.
  • The default python path can be overridden using the --python_path bazel flag. This patch does not break this behaviour.
  • A test exposing the bug is also provided. (This test has been written by @Profpatsch, thanks :) )
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 nox --run "nox-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.

@Profpatsch
Copy link
Member

Pushed a slight correction:

  • fix the missing callPackage (meta is not evaluated by default, so the missing symbol did not throw an error).
  • Rename the test file to use more conventional -.
  • allow substitutes in runLocal

@picnoir picnoir mentioned this pull request Jan 15, 2019
Closed
@picnoir @Profpatsch
bazel: fix python stub paths.
da8e049 
Since the 0.21 upgrade, the host `$PATH` is not forwarded anymore by
default to the sandboxes in charge to realize Bazel actions. This
default change broke the `py_binary` rule among other things.

Every python binary is wrapped in a stub in charge to setup the
execution environment. Currently, this stub's shebang points to a
`/usr/bin/env python` which cannot be resolved with the current
`$PATH`.
This results in breaking any build pipeline requiring the use of
python at some point. On top of the incorrect shebang, the stub
template is unable to find the actual python binary using
`SearchPath`.

This PR fixes those two things by re-writing the stub template shebang
to the actual python binary and by substituting the faulty default
python binary lookup to the right one.
@Profpatsch
Copy link
Member

@GrahamcOfBorg build bazel.tests

@Profpatsch
Copy link
Member

Awesomesauce. Next step is to add tests for all other built-in languages as well. I’ll open an issue for that.

@Profpatsch Profpatsch merged commit 5700473 into NixOS:master Jan 15, 2019
@Profpatsch Profpatsch mentioned this pull request Jan 15, 2019
Closed
orivej-nixos pushed a commit that referenced this pull request Jan 16, 2019
@orivej
bazel: fix patches after #53988
052db93
@picnoir picnoir mentioned this pull request Jan 22, 2019
Merged
10 tasks
@abbradar
Copy link
Member

@NinjaTrappeur I encountered problems with this patch and Python 2 as part of #64716 and partially reverted the fix (we now use PATH for finding Python to run). Strangely this didn't break anything, tests run fine and packages build. Can you check out 9ed16da? What do I miss?

@abbradar abbradar mentioned this pull request Jul 16, 2019
Merged
10 tasks
@Profpatsch
Copy link
Member

@abbradar Can you open an issue and ping me in it?

@picnoir picnoir deleted the bazel-python-fix branch December 11, 2019 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Profpatsch Profpatsch Awaiting requested review from Profpatsch Profpatsch is a code owner

Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@picnoir @Profpatsch @abbradar @GrahamcOfBorg