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.

Sign up for GitHub

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

WIP: wrapPython: only inject Python code, don't wrap with makeWrapper #58174

Closed
wants to merge 1 commit into from
Closed

WIP: wrapPython: only inject Python code, don't wrap with makeWrapper #58174

wants to merge 1 commit into from

Conversation

catern
Copy link
Contributor

@catern catern commented Mar 23, 2019

Motivation for this change

This makes Python executables (such as meson) which look at sys.argv[0] to find the currently executing script, work fine without special-casing anything.

See #58130

Still to resolve:

  • We're unconditionally adding -s to the Python shebang to disable user site reading; we can either merge that option with others, or fall back to using a shell script if there are already options in the shebang.
  • I removed makeWrapper entirely to demonstrate the potential, but we should probably still have makeWrapper be used if makeWrapperArgs is passed
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.

@catern catern requested a review from FRidh as a code owner March 23, 2019 14:51
@catern catern mentioned this pull request Mar 23, 2019
Open
@FRidh
Copy link
Member

FRidh commented Mar 23, 2019

This is essentially a continuation of #17749.

@FRidh
Copy link
Member

FRidh commented Mar 23, 2019

About PYTHONNOUSERSITE, #51641. I suggest making -s optional. In case makeWrapperArgs is not empty, create a wrapper, otherwise, don't. Maybe in time we can deprecate it. With these changes done we can create a job and test it.

FRidh
FRidh reviewed Mar 23, 2019
View reviewed changes
pkgs/development/interpreters/python/wrap-python.nix
@@ -29,14 +29,14 @@ makeSetupHook {
'';

# This preamble does two things:
# * Sets argv[0] to the original application's name; otherwise it would be .foo-wrapped.
# Python doesn't support `exec -a`.
# * Adds all required binary directories to PATH
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Append, prepend ?

@stale
Copy link

stale bot commented Jun 1, 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 1, 2020
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Oct 3, 2020
@ryantm ryantm marked this pull request as draft October 23, 2020 03:08
@siraben
Copy link
Member

siraben commented Apr 1, 2021 •
edited

Still important to me. What's the status of this PR?

@catern
Copy link
Contributor Author

catern commented Apr 1, 2021

Alas, I haven't done any further work on this, and at this point I probably won't, so I'll go ahead and close this. But I would love to see something like this merged - I still have code which would benefit from this.

@catern catern closed this Apr 1, 2021
@mkenigs
Copy link
Contributor

mkenigs commented Apr 1, 2021

What's still left to be done on what you had written?

@catern
Copy link
Contributor Author

catern commented Apr 1, 2021

IIRC I thought the change was essentially good in its current state (although of course it would be changed some in review). So if someone wanted to resurrect the change, I think it would just be a matter of rebasing, updating for @FRidh 's comments and opening a new PR. Also you probably would want to target the change at staging, and do some testing of packages that are affected by this which are mentioned in the linked issues (like qutebrowser).

@mkenigs
Copy link
Contributor

mkenigs commented Apr 1, 2021

Ok thanks! Didn't quite understand what you said about merging -s. Looks like that needs to be updated to respect permitUserSite, but are there ever other arguments passed to the shebang?

@catern
Copy link
Contributor Author

catern commented Apr 1, 2021

I'm not sure - there might not be any other argument passed to the shebang in current Nixpkgs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FRidh FRidh FRidh left review comments

@rasendubi rasendubi Awaiting requested review from rasendubi

Assignees
No one assigned
Labels
2.status: merge conflict 2.status: work-in-progress 6.topic: python 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@catern @FRidh @siraben @mkenigs @ryantm @samueldr @GrahamcOfBorg