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

stdenv: set SOURCE_DATE_EPOCH to a value python supports #89794

Merged
merged 1 commit into from Aug 1, 2020

Conversation

@Mic92
Copy link
Contributor

Mic92 commented Jun 8, 2020

in nix-shell this value breaks the build because python's
packaging refuses to build timestamps that date before 1980.

Motivation for this change

I am surprised this has been not done before... Tell me if I miss anything here.
cc @FRidh @jonringer

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.
in nix-shell this value breaks the build because python's
packaging refuses to build timestamps that date before 1980.
@Mic92 Mic92 requested review from Ericson2314 and matthewbauer as code owners Jun 8, 2020
@Mic92
Copy link
Contributor Author

Mic92 commented Jun 8, 2020

If there are no objections I would also fix the python docs before merging this.

@jonringer
Copy link
Contributor

jonringer commented Jun 8, 2020

This is handled for python packages using the ensureNewerSourcesForZipFilesHook, should be used by default if buildPython{Packages,Application} is used as the mkDerivation helper.

@jonringer
Copy link
Contributor

jonringer commented Jun 8, 2020

I think this has pretty big ramifications from a philosophical / purist perspective.

cc @edolstra

EDIT: also, this opens of the door to having to support any new archive format which may or may not support a given date range.

@Mic92
Copy link
Contributor Author

Mic92 commented Jun 8, 2020

This is handled for python packages using the ensureNewerSourcesForZipFilesHook, should be used by default if buildPython{Packages,Application} is used as the mkDerivation helper.

Well. it is sort of annoying so and it is not added when you add python to buildInputs.

@Mic92
Copy link
Contributor Author

Mic92 commented Jun 8, 2020

I think this has pretty big ramifications from a philosophical / purist perspective.

I don't get this point. Please backup with technical reasons.

@Mic92
Copy link
Contributor Author

Mic92 commented Jun 8, 2020

EDIT: also, this opens of the door to having to support any new archive format which may or may not support a given date range.

I don't see any reason not to change it again if we have good reason to. It does not comes with any cost if time it with another stdenv rebuild.

@jonringer
Copy link
Contributor

jonringer commented Jun 8, 2020

I don't get this point. Please backup with technical reasons.

I'm definitely not the best to comment on this. cc @FRidh

Trust me, I think it's annoying as well. I've just gotten used to including the hook to all my environments.

@FRidh
Copy link
Member

FRidh commented Jun 13, 2020

See earlier #60446

@Mic92
Copy link
Contributor Author

Mic92 commented Jun 14, 2020

I don't agree that we have to switch nix store dates. In a nix-shell it is enough to have SOURCE_DATE_EPOCH with the value given here set for python wheels to be build successfully. If that is all arguments against changing this value I would like to proceed to merge it.

@FRidh
Copy link
Member

FRidh commented Jun 14, 2020

If Python packaging using Nix needs it, then it needs to propagate the hook. Just make sure you propagate from the right tools.

@Mic92
Copy link
Contributor Author

Mic92 commented Jun 14, 2020

If Python packaging using Nix needs it, then it needs to propagate the hook. Just make sure you propagate from the right tools.

So should we added it to python's setup hook?

@FRidh
Copy link
Member

FRidh commented Jun 14, 2020

The interpreter does not do anything with wheels. The logical place would be the tools consuming or producing the wheels. When again does the issue occur, when building or installing wheels? If I am correct it is when building wheels. Thus the PEP 517 backends should propagate the hook. setuptools, flit-core, mesonpep517, poetry.

@Mic92
Copy link
Contributor Author

Mic92 commented Jun 14, 2020

The interpreter does not do anything with wheels. The logical place would be the tools consuming or producing the wheels. When again does the issue occur, when building or installing wheels? If I am correct it is when building wheels. Thus the PEP 517 backends should propagate the hook. setuptools, flit-core, mesonpep517, poetry.

Still this is bad UX. If someone includes python in buildInputs and uses venv to create a virtual environment, they just expect it to work and not having to include additional hooks. So why not set SOURCE_DATE_EPOCH to a reasonable value in the first place. There has been no technical reason given against it so far.

@FRidh
Copy link
Member

FRidh commented Jun 14, 2020

That's just because you use nix-shell, which gives an environment corresponding to a build environment. Using it for different purposes is not what it is meant for. Yes, its a convenient tool to use, we do it all the time, but you should be aware of its limitations.

@Mic92
Copy link
Contributor Author

Mic92 commented Jun 14, 2020

That's just because you use nix-shell, which gives an environment corresponding to a build environment. Using it for different purposes is not what it is meant for. Yes, its a convenient tool to use, we do it all the time, but you should be aware of its limitations.

nix-shell is the only time available at the time for build environments in nix. You cannot even use a C compiler properly without it because there is no other way to build include/library paths. Still you have not answered the question why we cannot change this value.

@FRidh
Copy link
Member

FRidh commented Jun 14, 2020

I don't know if it can or cannot be changed, and suggest you ask around further in the community, considering this is the stdenv.

@Mic92 Mic92 requested review from vcunat, adisbladis and edolstra Jun 14, 2020
@timokau
Copy link
Member

timokau commented Jun 14, 2020

I agree with @Mic92. The python issue is a major annoyance with a non-trivial fix. We can work around it with little to no cost. The fix isn't perfect, but strictly better than the status quo.

@Mic92 Mic92 merged commit f6a30fc into NixOS:staging Aug 1, 2020
17 of 18 checks passed
17 of 18 checks passed
stdenv, stdenv.passthru.tests on x86_64-darwin
Details
stdenv.passthru.tests on x86_64-darwin No attempt
Details
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="bc4927a"; rev="bc4927a5263aef5f418e81dc97c23bbf57f23d81"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="bc4927a"; rev="bc4927a5263aef5f418e81dc97c23bbf57f23d81"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="bc4927a"; rev="bc4927a5263aef5f418e81dc97c23bbf57f23d81"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="bc4927a"; rev="bc4927a5263aef5f418e81dc97c23bbf57f23d81"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="bc4927a"; rev="bc4927a5263aef5f418e81dc97c23bbf57f23d81"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="bc4927a"; rev="bc4927a5263aef5f418e81dc97c23bbf57f23d81"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="bc4927a"; rev="bc4927a5263aef5f418e81dc97c23bbf57f23d81"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
stdenv, stdenv.passthru.tests on aarch64-linux Success
Details
stdenv, stdenv.passthru.tests on x86_64-linux Success
Details
@Mic92 Mic92 deleted the Mic92:source-date-epoch branch Aug 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.