Adding tests to ensure pip install is reproducible

[FRidh]

Environment

• Nixpkgs at 9a4d723e436d8c1b94782dbff6d2d8dd4bf5dce5

• pip version: 20.0.2
• Python version: CPython 3.7
• OS: Any Linux OS using Nix builds

Description
Using pip at 19.3.1 building of Python packages was reproducible, however, since we upgraded to 20.0.2 that is no longer the case (NixOS/nixpkgs#81441). The issue is that bytecode now refers to a unpacked-wheel folder (see e.g. https://r13y.com/diff/b03e875784ba16e60ec59d1d5e720b13fc7ae1311edf409b819b0b264acd5393-aba2cac73c5234af8fbf694100bc631800ca397ad7afeed335d38895322a7c71.html).

The function for this unpacked-wheel was added in #7483, I have not bisected this any further though cc @chrahunt @raboof

Expected behavior
Reproducible build.

How to Reproduce

[chrahunt]

This makes sense if during the build you were specifying --build-dir. This should be resolved by #6030, since we will byte-compile the .py files in-place.

[FRidh]

We build wheels using

python setup.py bdist_wheel

and install them using

python -m pip install ./*.whl --no-index --prefix="$out" --no-cache $pipInstallFlags --build tmpbuild

[chrahunt]

Thanks. --build would have forced pip to use the same directory for the initial wheel extraction, assuming that directory was always the same during installation. Then since we do the byte-compilation on the extraction directory (instead of the install directory), it would have had the same embedded directory path each time.

A straightforward way to fix this without requiring too much change is to recurse over the unpacked wheel directory explicitly and use py_compile.compile with a dfile parameter that matches the expected install directory.

[chrahunt]

This should be fixed by #8541, however we will need tests before calling this done. Tests would look like:

1. create a wheel in-memory that contains some .py files (with tests.lib.wheel.make_wheel)
2. pip install that wheel with SOURCE_DATE_EPOCH set
3. hash all the files
4. uninstall that package, verify all files are gone
5. set tmpdir that pip would use to a different directory
6. pip install that wheel again
7. hash all the files and compare against previous digests

and would probably go in tests/functional/test_install_wheel.py (which can be referenced for some of the other steps).

[pfmoore]

pip install that wheel with SOURCE_DATE_EPOCH set

Am I missing something here? There's no mention of SOURCE_DATE_EPOCH in the pip codebase, tests, or documentation. So I don't think we should be making any commitments (much less mandating via tests) of how pip will behave if that's set.

[chrahunt]

It's not us, but the standard library. If we're using either of compileall.compile_file or py_compile.compile it should be handled for us automatically.

I don't have a stake in this, so I won't drive the discussion more than that. Anyone can feel free to pick this up and put time into it. :)

[pfmoore]

It's not us, but the standard library.

Ah, OK. In which case, I'd say that pip's test should simply be that we pass None to the stdlib function (and hence that we follow stdlib behaviour). We shouldn't test that the stdlib does what it says it will do.

If, however, we want to document for pip what guarantees we give around reproducibility, then yes we should have a test that ensures we deliver that behaviour. But I'd say document first, then test - we don't need any more tests that check undocumented behaviour 🙁

I don't have a stake in this

Nor do I, beyond not wanting to be expected to support a behaviour that I don't understand and personally have no need for. So I'll say no more for now. As you say, someone who has an interest in this can define what they want and push the discussion on that basis.

[nanonyme]

@pfmoore if PR was made that would make argument of said compile functions None to use standard library convention instead, would it be accepted? This is where it's documented https://docs.python.org/3/whatsnew/3.7.html#py-compile

[nanonyme]

The end result is essentially checked hash so bytecode is still validated against source for changes but not based on mtime but instead through hash.

[nanonyme]

Looks like https://github.com/pypa/pip/blob/master/src/pip/_vendor/distlib/util.py#L595 is fine as is. It will result in whatever standard library uses.

[nanonyme]

Similarly https://github.com/pypa/pip/blob/master/src/pip/_internal/operations/install/wheel.py#L715 looks fine as is. The default is checked hash assuming Python works as intended.

[nanonyme]

Any chance the title could be reflected that this issue is not in fact about fixing pip but adding tests? :)

[FRidh]

I can confirm the issue is indeed resolved with 20.2. Thank you. It would be great to have tests to avoid this issue from occurring again.

[pradyunsg]

PRs adding tests are welcome!

[nanonyme]

Uagh. I just read through a reproducible report out of our build system and apparently without the nixos patch pip using non-deterministic pip-req-build path results in eg in debuginfo for cython ending up with these paths hardcoded. I think the core thing here is that you can't externally affect the seed of mkdtemp. (which is not really a problem in pip but in CPython)

[nanonyme]

There was a bug open about this https://bugs.python.org/issue32276 but it was rejected. IMHO it's silly to blame everything around CPython about not being reproducible when the source of unreproducibility is in CPython.