python.pipInstallHook: avoid producing wrong direct_url.json file #232414

yajo · 2023-05-17T12:19:32Z

Fix problem detected in #229472 (comment).

Since bff6c67, now pname is expected to match the name of the package that is built by Python standard tools.

Description of changes

Things done

FRidh · 2023-05-17T12:24:36Z

hmm, yes, I am not sure whether we can really rely on pname. We need to test this and merge after branch-off. If everything is working well we can always choose to backport to stable.

vcunat · 2023-05-17T12:25:13Z

This was discussed in https://matrix.to/#/#staging:nixos.org even before the revert happened, but it doesn't solve the whole problem and it seems too late for such changes in 23.05.

vcunat · 2023-05-17T12:27:00Z

staging at this point won't reach 23.05 anymore, so it could be used already.

vcunat · 2023-05-17T12:28:25Z

For reference, multiple python packages from this list look like suffering from this, at a glance:
https://malob.github.io/nix-review-tools-reports/nixpkgs:staging-next/nixpkgs_staging-next_1795115.html

yajo · 2023-05-17T12:33:09Z

Something weird to me is that #229472 actually got green ✅ CI. There's probably something specific to nixpkgs that I'm misssing, but how am I supposed to know that a PR breaks things before merging (without toasting my laptop)? 🤔

vcunat · 2023-05-17T12:43:25Z

These kinds of changes cause rebuild of most packages. That's very expensive; way too much for just a CI. I could set up a separate jobset on hydra.nixos.org for this, so that it can get somewhat stabilized before merging. That is, assuming there's consensus that this is the way to go.

vcunat · 2023-05-17T12:46:31Z

Though maybe it's an overkill, if it's only expected to be such trivial renames and mostly for packages that fail themselves... in that case I'd expect it could be handled during staging-next stabilization (but not for 23.05 anyway).

yajo · 2023-05-18T07:53:02Z

Yes, that's basically the fix. You can see the details in #229472 (comment), but basically, instead of installing the whl file directly, we pass the directory name and the package name, and let python find the correct wheel. That's the official way of getting rid of the buggy direct_url.json. Thus, the pname must match the python package name.

I understand the need to be stable at this point, and I don't mind if this doesn't enter 23.05... but I'd just like to ask: How should I proceed now with this fix?

Thanks!

yajo · 2023-06-05T11:25:23Z

Now that 23.05 has born, I've rebased and cherry-picked bff6c67 again. This should be mergeable now.

If there's any way to know if this patch is breaking other packages, please tell me. Otherwise, this one should be ready to merge IIUC. Thanks!

yajo · 2023-07-18T10:05:19Z

Hi there!

This patch was approved some time ago and merged in #229472. Please read that description if you haven't, to understand how this is important.

Then it was reverted in #229472 (comment) due to the release rush.

I think there should be a review here now that there's plenty of time until the next release, so we can find and fix any possibly broken builds in the mean time.

Would somebody please do the review? Thanks!

SuperSandro2000 · 2023-07-18T13:33:39Z

I would like to include this in the python-updates run.

SuperSandro2000

I think we might now have naming conflicts if the internal package name is uppercase like for Twisted, see https:/NixOS/nixpkgs/blob/master/doc/languages-frameworks/python.section.md?plain=1#L1940-L1945

 ➜ nix build .#python3.pkgs.twisted.dist

 ➜ ls -lah result-dist
lrwxrwxrwx sandro users 75 B Tue Jul 18 18:42:11 2023  result-dist ⇒ /nix/store/bjc37hqna1g8374d6hp9aq4069dinva9-python3.10-twisted-22.10.0-dist

Also we should really add some helpful error messages otherwise people might be really confused if such a case happens for another package.

FRidh · 2023-07-18T18:58:39Z

Yes, I actually think this is not a good solution. As far as I recall there is no guarantee whatsoever that a package name on PyPI will match with a wheel name.

When installing many python packages, a `direct_url.json` file appeared in the lib directory. Example: ```sh ➤ nix build nixpkgs/29755fec55e58a315b517d431b2be261772f2b80#python3Packages.flask ➤ cat result/lib/python3.10/site-packages/Flask-2.2.3.dist-info/direct_url.json {"archive_info": {}, "url": "file:///build/Flask-2.2.3/dist/Flask-2.2.3-py3-none-any.whl"}⏎ ``` As you can see, that file contains a wrong reference to `/build`. In https://discuss.python.org/t/pep-610-usage-guidelines-for-linux-distributions/4012/4 there's an explanation on how to avoid this. Here, I'm implementing that change for nixpkgs. @moduon MT-1075

Fix problem detected in NixOS#229472 (comment). Since bff6c67, now pname is expected to match the name of the package that is built by Python standard tools.

This makes sure the problem we're fixing gets fixed.

As seen in NixOS#232414 (review), some packages get built to a different name than nixpkgs' standard pname. Those packages can now use `wheelPname` to get built. Added to Twisted, just to test it out.

yajo · 2023-07-19T11:07:18Z

I pushed some changes, please review again.

Now I have extended the audit tmpdir function. I always wondered why this wasn't detected here:

nixpkgs/pkgs/build-support/setup-hooks/audit-tmpdir.sh

Line 16 in 3fa4913

echo "checking for references to $TMPDIR/ in $dir..."

That test would break almost all python packages if it weren't because I push the fix here too. 😆 But you can run it if you want, to see how /build was slipping into the built packages.

Maybe it's not so problematic after all, I'm not sure. According to https://packaging.python.org/en/latest/specifications/direct-url/, it's just metadata. 🤷🏼‍♂️ But in general I think nothing from /build should land in the drv output.

I fixed a couple of derivations, including Twisted, making use of the new wheelPname attribute. Also I added a helpful error message.

SuperSandro2000 · 2023-07-19T13:27:36Z

pkgs/development/python-modules/twisted/default.nix


 disabled = pythonOlder "3.6";

 src = fetchPypi {
- pname = "Twisted";
+ pname = wheelPname;


Suggested change

pname = wheelPname;

pname = wheelName;

not sure if that is better or if we should rename the attr in fetchPypi

I think wheelPname is better because it is more intuitive that you need to put the value that you would be putting in pname and not in name. The wheel files that are generated include the pname and version (aka name), so here we really care about the pname part.

not sure if that is better or if we should rename the attr in fetchPypi

Most times the value will be the same. I can add that attribute there, and assert you're setting only one of both. Since those derivations are FODs, changes there shouldn't produce mass rebuilds.

SuperSandro2000 · 2023-07-19T13:28:06Z

pkgs/development/interpreters/python/hooks/pip-install-hook.sh

@@ -22,3 +25,20 @@ if [ -z "${dontUsePipInstall-}" ] && [ -z "${installPhase-}" ]; then
 echo "Using pipInstallPhase"
 installPhase=pipInstallPhase
 fi
+
+
+pipAuditTmpdir() {


There is also a tool called pip-audit which makes this a bit confusing

I was trying to follow the terminology from here because it's mostly extending that safety net:

nixpkgs/pkgs/build-support/setup-hooks/audit-tmpdir.sh

Line 12 in 3fa4913

auditTmpdir() {

But just tell me what name you prefer, I can change it, no problem.

FRidh · 2023-07-21T08:49:09Z

We should take a step back here and look at what the problem is that we're trying to solve.

PEP 610 describes how frontends should create a direct_url.json file when installing wheels. This file is causing trouble for us, because it can contain references we don't like. These references aren't really used, they're for provenance. We care about provenance, but provenance is already part of our Nix expressions. So, for as far I can tell, we may as well delete these files.

The only case we're interested in them, is for PEP 662 editable installations. But that's only when using the phase specific for editable installations. For regular installations, we can still delete the file.

Thus, I suggest we always remove the file from the dist-info folder.

yajo · 2023-07-21T10:37:00Z

It's another possible fix. It just felt more safe for me to let do pip its stuff without interfering.

yajo · 2023-09-05T11:55:46Z

Does this still make sense after #248866 was merged? 🤔

github-actions bot added the 6.topic: python label May 17, 2023

yajo mentioned this pull request May 17, 2023

python.pipInstallHook: avoid producing wrong direct_url.json file #229472

Merged

12 tasks

ofborg bot requested a review from costrouc May 17, 2023 15:07

ofborg bot added 10.rebuild-darwin: 501+ 10.rebuild-darwin: 501-1000 10.rebuild-linux: 501+ 10.rebuild-linux: 1001-2500 labels May 17, 2023

yajo force-pushed the fix-python-lz4 branch from 91ba153 to 14de361 Compare June 5, 2023 11:24

yajo requested review from FRidh and jonringer as code owners June 5, 2023 11:24

ofborg bot added 10.rebuild-darwin: 5001+ 10.rebuild-linux: 5001+ and removed 10.rebuild-darwin: 501-1000 10.rebuild-linux: 1001-2500 labels Jun 5, 2023

yajo changed the title ~~python310Packages.lz4: fix build~~ python.pipInstallHook: avoid producing wrong direct_url.json file Jul 18, 2023

yajo force-pushed the fix-python-lz4 branch from 14de361 to 7d6ace2 Compare July 18, 2023 10:01

ofborg bot added the 10.rebuild-darwin-stdenv label Jul 18, 2023

SuperSandro2000 mentioned this pull request Jul 18, 2023

Python updates 2023-07-18 #244135

Merged

SuperSandro2000 requested changes Jul 18, 2023

View reviewed changes

yajo added 3 commits July 19, 2023 11:00

python310Packages.lz4: fix build

9bd6625

Fix problem detected in NixOS#229472 (comment). Since bff6c67, now pname is expected to match the name of the package that is built by Python standard tools.

python: extend audit tmpdir for direct_url.json files

6a4bec7

This makes sure the problem we're fixing gets fixed.

yajo force-pushed the fix-python-lz4 branch from 7d6ace2 to 4fb051b Compare July 19, 2023 10:01

python.pipInstallHook: support wheelName

0c2bbc4

As seen in NixOS#232414 (review), some packages get built to a different name than nixpkgs' standard pname. Those packages can now use `wheelPname` to get built. Added to Twisted, just to test it out.

yajo force-pushed the fix-python-lz4 branch from 4fb051b to 0c2bbc4 Compare July 19, 2023 11:01

ofborg bot requested a review from SuperSandro2000 July 19, 2023 11:59

SuperSandro2000 reviewed Jul 19, 2023

View reviewed changes

wegank added 2.status: stale https:/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: merge conflict labels Mar 19, 2024

stale bot removed the 2.status: stale https:/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024

wegank added the 2.status: stale https:/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

python.pipInstallHook: avoid producing wrong direct_url.json file #232414

python.pipInstallHook: avoid producing wrong direct_url.json file #232414

yajo commented May 17, 2023

FRidh commented May 17, 2023

vcunat commented May 17, 2023

vcunat commented May 17, 2023

vcunat commented May 17, 2023

yajo commented May 17, 2023

vcunat commented May 17, 2023

vcunat commented May 17, 2023

yajo commented May 18, 2023

yajo commented Jun 5, 2023

yajo commented Jul 18, 2023

SuperSandro2000 commented Jul 18, 2023

SuperSandro2000 left a comment •

edited

Loading

FRidh commented Jul 18, 2023

yajo commented Jul 19, 2023

SuperSandro2000 Jul 19, 2023

yajo Jul 21, 2023

yajo Jul 21, 2023

SuperSandro2000 Jul 19, 2023

yajo Jul 21, 2023

FRidh commented Jul 21, 2023

yajo commented Jul 21, 2023

yajo commented Sep 5, 2023

python.pipInstallHook: avoid producing wrong direct_url.json file #232414

Are you sure you want to change the base?

python.pipInstallHook: avoid producing wrong direct_url.json file #232414

Conversation

yajo commented May 17, 2023

Description of changes

Things done

FRidh commented May 17, 2023

vcunat commented May 17, 2023

vcunat commented May 17, 2023

vcunat commented May 17, 2023

yajo commented May 17, 2023

vcunat commented May 17, 2023

vcunat commented May 17, 2023

yajo commented May 18, 2023

yajo commented Jun 5, 2023

yajo commented Jul 18, 2023

SuperSandro2000 commented Jul 18, 2023

SuperSandro2000 left a comment • edited Loading

Choose a reason for hiding this comment

FRidh commented Jul 18, 2023

yajo commented Jul 19, 2023

SuperSandro2000 Jul 19, 2023

Choose a reason for hiding this comment

yajo Jul 21, 2023

Choose a reason for hiding this comment

yajo Jul 21, 2023

Choose a reason for hiding this comment

SuperSandro2000 Jul 19, 2023

Choose a reason for hiding this comment

yajo Jul 21, 2023

Choose a reason for hiding this comment

FRidh commented Jul 21, 2023

yajo commented Jul 21, 2023

yajo commented Sep 5, 2023

SuperSandro2000 left a comment •

edited

Loading