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

pythonPackages.pip: make reproducible #102222

Merged
merged 1 commit into from Oct 31, 2020
Merged

Conversation

zimbatm
Copy link
Member

@zimbatm zimbatm commented Oct 31, 2020

The previous attempt wasn't covering all of the bases. It relied on
invoking that pip-install-hook, and didn't apply to pip itself.

The core issue is that the generated .pyc files embed some of the
temporary paths, which are randomly generated. See
https://r13y.com/diff/bf8c3ca3148ebff9ecf41f294cc60b9f209c006d49699e356969ff32d736f1c6-8806a7cca91fdd300e48736bfcd57c4d0b54c1cc2fd61609f35143170862b59c.html

In this new attempt, the approach is to patch the TempFile
implementation directly, so that it creates stable temporary
directories. We also assume that if SOURCE_DATE_EPOCH is set, we are in
a scenario where reproducible builds are desirable and enter that
branch.

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@zimbatm
Copy link
Member Author

zimbatm commented Oct 31, 2020

Sorry for the notification spam.

@FRidh is there any package beyond pip that is critical to test?

Here are some packages that are now reproducible:

  • pythonPackages.bootstrapped-pip
  • pythonPackages.pip
  • pythonPackages.pytest
  • pythonPackages.setuptools
  • pythonPackages.wheel
  • python3Packages.bootstrapped-pip
  • python3Packages.pip
  • python3Packages.pytest # builds but is not reproducible, due to another issue
  • python3Packages.setuptools
  • python3Packages.wheel

@FRidh
Copy link
Member

FRidh commented Oct 31, 2020

pytest is likely due to its own cache, although that we do handle.

Please refer in the code to the upstream issue pypa/pip#7808. Same issue was also noticed by Bazel people pypa/pip#6505 (comment).

The previous attempt wasn't covering all of the bases. It relied on
invoking that pip-install-hook, and didn't apply to pip itself.

The core issue is that the generated .pyc files embed some of the
temporary paths, which are randomly generated. See
https://r13y.com/diff/bf8c3ca3148ebff9ecf41f294cc60b9f209c006d49699e356969ff32d736f1c6-8806a7cca91fdd300e48736bfcd57c4d0b54c1cc2fd61609f35143170862b59c.html

In this new attempt, the approach is to patch the TempFile
implementation directly, so that it creates stable temporary
directories. We also assume that if SOURCE_DATE_EPOCH is set, we are in
a scenario where reproducible builds are desirable and enter that
branch.

See also pypa/pip#7808
@zimbatm
Copy link
Member Author

zimbatm commented Oct 31, 2020

@FRidh fixed. Thanks for the review!

@zimbatm zimbatm merged commit c409f69 into NixOS:staging Oct 31, 2020
@zimbatm zimbatm deleted the python-pip-r13y branch October 31, 2020 20:24
@jonringer
Copy link
Contributor

should we upstream this? pip should respect SOURCE_DATE_EPOCH

@FRidh
Copy link
Member

FRidh commented Nov 1, 2020

should we upstream this? pip should respect SOURCE_DATE_EPOCH

This patch is a hack. While the patch is for reproducibility, SOURCE_DATE_EPOCH has nothing to do with it, and is solely used here to indicate "we want it reproducible". For that reason the previous patch had NIX_PIP_INSTALL_TMPDIR.

@zimbatm
Copy link
Member Author

zimbatm commented Nov 1, 2020

Semantically speaking SOURCE_DATE_EPOCH is not entirely correct. A better fix would be for upstream to fix their build so that the build directories are not included in the .pyc files.

@flokli
Copy link
Contributor

flokli commented Nov 1, 2020

This seems to have regressed things pretty early in the chain, nix-build -A nss now fails:

I was wong. it's #102156.

@jonringer
Copy link
Contributor

This patch is a hack. While the patch is for reproducibility, SOURCE_DATE_EPOCH has nothing to do with it, and is solely used here to indicate "we want it reproducible"

I stopped reading after the if statement. Yea, this is a hack, but an improvement.

@FRidh
Copy link
Member

FRidh commented Nov 5, 2020

This triggered failures in case of format = "pyproject"; and this will probably block staging-next.

It seems as if the build phase is run twice, in which case its not because of a bug in this patch, but because of a mistake in the hook pip-build-hook.sh. a temporary directory is created twice, but I don't yet know why.

Grep for format = "pyproject"; to find projects. Easiest to test is probably python3.pkgs.flit-core.

Building wheels for collected packages: pipdate
ERROR: Exception:
Traceback (most recent call last):
  File "/nix/store/86jpajr49njgc228c5dqk0k4haimn95s-python3.8-pip-20.1.1/lib/python3.8/site-packages/pip/_internal/cli/base_command.py", line 188, in _main
    status = self.run(options, args)
  File "/nix/store/86jpajr49njgc228c5dqk0k4haimn95s-python3.8-pip-20.1.1/lib/python3.8/site-packages/pip/_internal/cli/req_command.py", line 185, in wrapper
    return func(self, options, args)
  File "/nix/store/86jpajr49njgc228c5dqk0k4haimn95s-python3.8-pip-20.1.1/lib/python3.8/site-packages/pip/_internal/commands/wheel.py", line 169, in run
    build_successes, build_failures = build(
  File "/nix/store/86jpajr49njgc228c5dqk0k4haimn95s-python3.8-pip-20.1.1/lib/python3.8/site-packages/pip/_internal/wheel_builder.py", line 285, in build
    wheel_file = _build_one(
  File "/nix/store/86jpajr49njgc228c5dqk0k4haimn95s-python3.8-pip-20.1.1/lib/python3.8/site-packages/pip/_internal/wheel_builder.py", line 190, in _build_one
    return _build_one_inside_env(
  File "/nix/store/86jpajr49njgc228c5dqk0k4haimn95s-python3.8-pip-20.1.1/lib/python3.8/site-packages/pip/_internal/wheel_builder.py", line 202, in _build_one_inside_env
    with TempDirectory(kind="wheel") as temp_dir:
  File "/nix/store/86jpajr49njgc228c5dqk0k4haimn95s-python3.8-pip-20.1.1/lib/python3.8/site-packages/pip/_internal/utils/temp_dir.py", line 138, in __init__
    path = self._create(kind)
  File "/nix/store/86jpajr49njgc228c5dqk0k4haimn95s-python3.8-pip-20.1.1/lib/python3.8/site-packages/pip/_internal/utils/temp_dir.py", line 187, in _create
    os.mkdir(path)
FileExistsError: [Errno 17] File exists: '/build/pip-wheel-immobile'

@FRidh FRidh mentioned this pull request Nov 5, 2020
10 tasks
@FRidh
Copy link
Member

FRidh commented Nov 5, 2020

Seems like the issue is resolved in pip 20.2, upgrading now and reverting the patches.

FRidh added a commit that referenced this pull request Nov 5, 2020
Reproducible builds of pyproject projects using pip is resolved.

Fixes pypa/pip#7808
Fixes #81441

The more recent c409f69 caused trouble
with pyproject troubles and had to be reverted anyway.
#102222 (comment)

Revert "pythonPackages.pip: make reproducible (#102222)"

This reverts commit c409f69.

Revert "python3Packages.pip: allow setting reproducible temporary directory via NIX_PIP_INSTALL_TMPDIR"

This reverts commit aedbade.
FRidh added a commit that referenced this pull request Nov 5, 2020
Reproducible builds of pyproject projects using pip is resolved.

Fixes pypa/pip#7808
Fixes #81441

The more recent c409f69 caused trouble
with pyproject troubles and had to be reverted anyway.
#102222 (comment)

Revert "pythonPackages.pip: make reproducible (#102222)"

This reverts commit c409f69.

Revert "python3Packages.pip: allow setting reproducible temporary directory via NIX_PIP_INSTALL_TMPDIR"

This reverts commit aedbade.
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/hydra-security-model-discussion/9801/12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants