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

scons: Fix a wrapping issue which overrides your PATH #61213

Closed
wants to merge 1 commit into from

Conversation

knedlsepp
Copy link
Member

This fixes the following situation:
If your PATH contains a python.withPackages interpreter and as part of
your SConstruct build you want to launch a python script in a
subprocess, which should run with '#!/usr/bin/env python', then it would
be problematic to have scons wrapped with an additional PATH pointing to
a different python interpreter. None of your withPackages-packages would
be importable.

In a sidenote: this will also remove the PYTHONNOUSERSITE argument. I'm not yet sure if this behavior is good or bad. So to whoever will be reviewing this, I'm especially interested in this part of the PR.

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.

This fixes the following situation:
If your PATH contains a python.withPackages interpreter and as part of
your SConstruct build you want to launch a python script in a
subprocess, which should run with '#!/usr/bin/env python', then it would
be problematic to have scons wrapped with an additional PATH pointing to
a different python interpreter. None of your withPackages-packages would
be importable.
@knedlsepp
Copy link
Member Author

knedlsepp commented May 9, 2019

@primeos: Could you maybe have a look?

@primeos
Copy link
Member

primeos commented May 11, 2019

@knedlsepp This is an interesting problem, thanks for the PR :)

To document things a bit on this PR I'll add an example of the current wrapper:

#! /nix/store/5p8qcz6i13ymgf80kimhx2g9pb5r5nia-bash-4.4-p23/bin/bash -e
export PATH='/nix/store/29xfvq39h5z7af90id0zaa4a09vgpsm7-python-2.7.16/bin:/nix/store/cc85cq9lvy89kcb0806gv3yhq0iawb45-scons-3.0.5/bin:/nix/store/2jh8bf3k21hkc7agik6idy5mjr2yljrx-python2.7-setuptools-41.0.1/bin'${PATH:+':'}$PATH
export PYTHONNOUSERSITE='true'
exec -a "$0" "/nix/store/cc85cq9lvy89kcb0806gv3yhq0iawb45-scons-3.0.5/bin/.scons-wrapped"  "${extraFlagsArray[@]}" "$@"

Which ensures that the following binaries are available in $PATH:

/nix/store/29xfvq39h5z7af90id0zaa4a09vgpsm7-python-2.7.16/bin:
2to3  idle  pdb  pdb2.7  pydoc	python	python2  python2.7  python2.7-config  python2-config  python-config  smtpd.py  smtpd.pyc  smtpd.pyo

/nix/store/2jh8bf3k21hkc7agik6idy5mjr2yljrx-python2.7-setuptools-41.0.1/bin:
easy_install  easy_install-2.7

/nix/store/cc85cq9lvy89kcb0806gv3yhq0iawb45-scons-3.0.5/bin:
scons  scons-3.0.5  scons-3.0.5.bat  scons.bat	scons-configure-cache  scons-configure-cache-3.0.5  sconsign  sconsign-3.0.5  scons-time  scons-time-3.0.5

TODO: Before we merge this PR we have to verify if any of these binaries are required in $PATH. If that's the case we could theoretically also suffix $PATH or e.g. only include the SCons path.

Theoretically adding additional modules should work via $PYTHONPATH (nix-shell should set that environment variable for you) as long as they're for Python 2.7.
@knedlsepp did you already try if that works (e.g. by running the build within nix-shell -I nixpkgs=. -p pythonPackages.XYZ)?
That solution would however force one to use Python 2.7 (speaking of that, we should also switch SCons to Python 3 at some point, but at least 3.7 isn't officially supported yet).

cc @FRidh (and @risicle who introduced dontWrapPythonPrograms): Do you have any insights/opinions on this? I'm unfortunately not that familiar with our Python infrastructure :o

@knedlsepp
Copy link
Member Author

knedlsepp commented May 11, 2019

Theoretically adding additional modules should work via $PYTHONPATH

I encountered a situation where this was not applicable for multiple reasons:

  • scons is wrapped using python2Packages, so if one wanted to call a python3 script as part of the build this would not work
  • I was dealing with a SConstruct file that did purposely ignore all environment variables except PATH

Both issues can be patched away for a nix-build of that particular package with the appropriate postPatch phase. However interactively using scons to build stuff requires applying these patches manually.
The patch I'm proposing would solve the issue at hand.
(There's actually also some projects I've found which require having scons running the same python version you want to build a project for, so I was thinking that we probably to have:
nixpkgs.python2Packages.scons, nixpkgs.python3Packagse.scons, nixpkgs.scons = nixpkgs.pythonPackages.scons)

@stale
Copy link

stale bot commented Jun 2, 2020

Thank you for your contributions.
This has been automatically marked as stale because it has had no activity for 180 days.
If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.
Here are suggestions that might help resolve this more quickly:

  1. Search for maintainers and people that previously touched the
    related code and @ mention them in a comment.
  2. Ask on the NixOS Discourse. 3. Ask on the #nixos channel on
    irc.freenode.net.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 2, 2020
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Oct 3, 2020
@stale
Copy link

stale bot commented Jun 4, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 4, 2021
@knedlsepp knedlsepp closed this Aug 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants