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.

Sign up for GitHub

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

Jump to bottom

fetchurl: warn on rev archives, resolves #32999 #48325

Merged
merged 1 commit into from Apr 24, 2019
Merged

fetchurl: warn on rev archives, resolves #32999 #48325

merged 1 commit into from Apr 24, 2019

Conversation

lukateras
Copy link
Member

Motivation for this change

Resolves #32999. cc @c0bw3b @orivej @vcunat

Usage example:

$ nix-build . -A all-cabal-hashes --substituters ''
these derivations will be built:
  /nix/store/kldqyijahq28d8jdyhk8zi0fnpd7frg4-70f02ad82349a18e1eff41eea4949be532486f7b.tar.gz.drv
building '/nix/store/kldqyijahq28d8jdyhk8zi0fnpd7frg4-70f02ad82349a18e1eff41eea4949be532486f7b.tar.gz.drv'...
warning: rev archives should use fetchzip instead

trying https://github.com/commercialhaskell/all-cabal-hashes/archive/70f02ad82349a18e1eff41eea4949be532486f7b.tar.gz
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   176    0   176    0     0    218      0 --:--:-- --:--:-- --:--:--   218
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-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)
  • Fits CONTRIBUTING.md.

@lukateras
Copy link
Member Author

@grahamc Has fetchzip vs fetchFromGit{Hub,Lab} debate seen any resolution so far?

Either way, I've applied your feedback. Now it prints warning: archives from GitHub revisions should use fetchFromGitHub, and a corresponding line for GitLab.

c0bw3b
c0bw3b approved these changes Oct 28, 2018
View reviewed changes
Copy link
Contributor

@c0bw3b c0bw3b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both fetchzip and fetchFromGitThing should continue to be usable in similar situations IMHO. That's not really an issue and it gives regular contributors some space for personal preferences.

The real issue is not letting someone use fetchurl and I think the warning message here is clear enough and to the point.
Basically saying "fetchurl no -> fetchFromGitHub/Lab yes" leaves no room for confusion.

The fetchzip alternative can still be mentioned later in a PR review process if relevant.

@lukateras
Copy link
Member Author

@grahamc Does it look good to you now?

@lukateras
Copy link
Member Author

@grahamc Ping!

@grahamc
Copy link
Member

grahamc commented Apr 24, 2019

@GrahamcOfBorg eval
@GrahamcOfBorg build hello.src

@grahamc grahamc merged commit b261534 into NixOS:master Apr 24, 2019
@grahamc
Copy link
Member

grahamc commented Apr 24, 2019

Thank you for the ping, sorry!

@kvtb
Copy link
Contributor

kvtb commented Feb 23, 2021 •
edited

This is totally misleading warning.

  1. fetchurl of "https://github.com/*/archive/*" keeps the tarball packed while fetchFromGitHub does unpack it
  2. as a side effect unpacking to Nix Store erases timestamps on the source files, breaking $SOURCE_DATE_EPOCH logic ($SOURCE_DATE_EPOCH is always 1 if src= points to a local directory #112595)

I could say that fetchurl behaves better in both aspects and should be preferred.

Anyway, fetchFromGitHub is not a replacement of fetchurl, they are much too different.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grahamc grahamc grahamc left review comments

@c0bw3b c0bw3b c0bw3b approved these changes

Assignees
No one assigned
Labels
6.topic: fetch 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

fetchurl: warn on GitHub/GitLab ref archives
5 participants
@lukateras @grahamc @kvtb @c0bw3b @GrahamcOfBorg