Revert "Unwrap python scripts when building an environment"

[SuperSandro2000]

This reverts commit 9611885 from #297628

Closes #301498

Description of changes

The sed does bad assumptions and breaks python scripts like pretalx-manager #302315 (https://github.com/pretalx/pretalx/blob/dd9f39179a918c9b943fb64da0797fae930ff170/src/manage.py#L2) or xonsh #301449

Tested in #302315 by setting dontWrapPythonPrograms = true which unbroke the script. Before import os was being deleted by sed in line 3.

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

---

Add a 👍 reaction to pull requests you find important.

[SuperSandro2000]

With just the first revert gobject-introspection didn't want to build and complained about missing python packages, so I assume we also need to revert the other python changes. As they wheren't done in proper commits, I needed to split the commit a bit.

[SamLukeYes]

#301449 might also need to be reverted if this PR is merged, or xonsh users will likely encounter #248978 again

[domenkozar]

If only two packages are broken it might make more sense to workaround the issue and properly fix it?

[LordGrimmauld]

If only two packages are broken it might make more sense to workaround the issue and properly fix it?

it is definitely more, i used git bisect and determined #301498 to be caused by #297628 as well. Its not just xonsh and some libs.

[FRidh]

We should revert the changes, make fixes where needed, create a separate test job, reiterate when needed, and then submit again to staging.

[SuperSandro2000]

If only two packages are broken it might make more sense to workaround the issue and properly fix it?

I think the unwrap logic is doing not always true assumptions and is currently hit or miss and can cause random breakages in undefined places.

[alyssais]

This has also broken mailman at runtime, as can be seen from its NixOS test.

[SuperSandro2000]

Since no one opposed reverting this with an alternative in the last 4 days, I think we should just merge this to fix wrapped applications.

@vcunat can we get this into staging-next to get this fixed faster or should we target staging?

[alyssais]

can we get this into staging-next to get this fixed faster or should we target staging?

There is no staging-next currently underway, so this should just be merged into staging before the next staging-next starts.

[vcunat]

It rebuilds darwin stdenv. You don't suggest making a separate staging-next iteration just for this change, right?

[SuperSandro2000]

It rebuilds darwin stdenv. You don't suggest making a separate staging-next iteration just for this change, right?

Na, we just merge it into staging and take it with that.

Also I've just added the 24.05 milestone, as this is release critical.

[ruro]

Since no one opposed reverting this with an alternative in the last 4 days, I think we should just merge this to fix wrapped applications.

Hi, I am not 100% sure about the xonsh issue, but I think that in the django/pretalx case, the issue is caused by the fact that .pretalx-manage-wrapped doesn't include a # -*- coding: utf-8 -*- line for some reason.

As quick fix, we can do something like:

sed \
    -e '/^#!\/nix\/store\//d' \
    -e '/^import sys;import site;import functools;sys\.argv\[0\] = /d' \
    ".$prg-wrapped" >> "$out/bin/$prg"

instead of

sed -e '1d' -e '3d' ".$prg-wrapped" >> "$out/bin/$prg"

This should be more robust to weird wrapper variations by only deleting the lines that start with #!/nix/store/ and import sys;import site;import functools;sys.argv[0] = instead of blindly expecting them to be the first and third line.

Of course, a more "proper" solution would probably be to avoid wrapping and unwrapping these scripts in the first place, but I am unsure about how that should be done. @cwp, I think, you mentioned in the original PR that you had an idea about that?

[SuperSandro2000]

Please explore properly fixing this in another PR. Getting this fixed is release critical, the new feature is not, and fixing this will cause a lot of rebuilds, hence we should get this definitely into the next staging-next cycle to not delay build failure fixing for the 24.05 release.

[SuperSandro2000]

Cross-posting from #303057 (comment) to for visibility:

Do we need to carry over e05a3ec (#303057) because of the update or can we drop that here, too?

[mweinelt]

Feel free to test that out. I don't know.

[ruro]

Please explore properly fixing this in another PR. Getting this fixed is release critical, the new feature is not, and fixing this will cause a lot of rebuilds, hence we should get this definitely into the next staging-next cycle to not delay build failure fixing for the 24.05 release.

Understandable. It's a bit unfortunate since I was hoping to get the fixed venv behaviour in 24.05, but oh well.

Are there any known/suspected affected packages apart from xonsh and pretalx? It's a bit hard to verify that the proposed fix works with just two examples and (seemingly?) no relevant tests.

[SuperSandro2000]

Are there any known/suspected affected packages apart from xonsh and pretalx? It's a bit hard to verify that the proposed fix works with just two examples and (seemingly?) no relevant tests.

pypy3 but that's generally not that well maintained.

I think this could affect all wrapped scripts that are not generated by the build tooling and are just copied to $out/bin (eg https://github.com/NixOS/nixpkgs/blob/master/pkgs/by-name/pr/pretalx/package.nix#L153)

[mweinelt]

pypy3 but that's generally not that well maintained.

The only way to make it work is by recursing into the package set.

[LordGrimmauld]

pypy3 on nixos is itself a mess, as it makes some very specific assumptions about argv and python path env var at build time. I am honestly amazed it built at all before... That said: such a change as the here-reverted PR made might warrant revisiting some python applications that made questionable assumptions before. #297628 having broken pypy3 doesn't mean pypy3 had reasonable build logic even before it was blown to pieces. The installation logic is quite involved as it goes through multiple iterations of wrapping and compiling, it doesn't just build all at once.

[alyssais]

Are there any known/suspected affected packages apart from xonsh and pretalx?

I also mentioned Mailman in this thread.

[jonringer]

Getting this fixed is release critical, the new feature is not, and fixing this will cause a lot of rebuilds, hence we should get this definitely into the next staging-next cycle to not delay build failure fixing for the 24.05 release.

Agree with this sentiment. Getting it into staging now should make it available before ZHF.

[jonringer]

Going on the recommendation of @FRidh

We should revert the changes, make fixes where needed, create a separate test job, reiterate when needed, and then submit again to staging.

I'm going to merge this into staging. There's not currently a staging-next job open to expedite this, so the only other option would be to merge to master... or take more time for a more complete solution (which I think would be better tackled in a separate jobset in a separate PR).

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/how-to-build-python-virtualenv-with-packages-provided-by-python3-withpackages/24766/21

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/how-to-build-python-virtualenv-with-packages-provided-by-python3-withpackages/24766/16