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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fetchFromGitHub: re-fetch upon version changes #294068

Draft
wants to merge 3 commits into
base: staging
Choose a base branch
from
Draft

fetchFromGitHub: re-fetch upon version changes #294068

wants to merge 3 commits into from

Conversation

ShamrockLee
Copy link
Contributor

@ShamrockLee ShamrockLee commented Mar 7, 2024 •
edited

Description of changes

This PR mitigates the development issue of irrelevent FOD hashes after version bump/version overriding, by making rev part of the store path through the default value of the name attribute.

Depends on #294329 and #294334. Once they're merged, there will be only one commit and two changed files (including the release note) in this PR.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 馃憤 reaction to pull requests you find important.

@ShamrockLee ShamrockLee changed the base branch from master to staging March 7, 2024 15:21
@ShamrockLee ShamrockLee mentioned this pull request Mar 7, 2024
Open
@ShamrockLee ShamrockLee changed the base branch from staging to master March 7, 2024 16:01
@ShamrockLee ShamrockLee changed the title fetchFromGitHub: support version and re-fetch upon version/rev changes fetchFromGitHub: support version and re-fetch upon version changes Mar 7, 2024
@ShamrockLee
Copy link
Contributor Author

@ofborg build archivebox python311Packages.jwt python312Packages.jwt

@ShamrockLee ShamrockLee changed the base branch from master to staging March 7, 2024 21:28
@ShamrockLee
Copy link
Contributor Author

@ofborg build cowsay

@ShamrockLee
Copy link
Contributor Author

@ofborg build nixdoc

@ShamrockLee ShamrockLee force-pushed the fetchgithub-refetch branch 2 times, most recently from 08f6449 to 3b8b903 Compare March 8, 2024 17:37
@ShamrockLee ShamrockLee changed the title fetchFromGitHub: support version and re-fetch upon version changes fetchFromGitHub: re-fetch upon version changes Mar 8, 2024
@ShamrockLee
Copy link
Contributor Author

Let's limit the scope of this PR and avoid changing the interface too much.

@ShamrockLee ShamrockLee force-pushed the fetchgithub-refetch branch 3 times, most recently from 408e863 to f893a82 Compare March 8, 2024 18:27
@ShamrockLee ShamrockLee mentioned this pull request Mar 8, 2024
Merged
13 tasks
@ShamrockLee
Copy link
Contributor Author

nixpkgs-review shows no evaluation error. The ofborg-eval-lib-tests is due to failure to bootstrap its own tool chain.

I couldn't find a way to reproduce the build failure as well. Even llvm builds alright locally.

@ghost
Copy link

ghost commented Mar 10, 2024

nixpkgs-review shows no evaluation error. The ofborg-eval-lib-tests is due to failure to bootstrap its own tool chain.

I couldn't find a way to reproduce the build failure as well. Even llvm builds alright locally.

ofborg-eval-lib-tests failure 
   ran test tests/add.sh... [FAIL]
    ++ nix-store --add ./dummy
    + path1=/build/nix-test/add/store/n7knm3rma6fckrfwkx2vkiql3d1rgs1i-dummy
    + echo /build/nix-test/add/store/n7knm3rma6fckrfwkx2vkiql3d1rgs1i-dummy
    /build/nix-test/add/store/n7knm3rma6fckrfwkx2vkiql3d1rgs1i-dummy
    ++ nix-store --add-fixed sha256 --recursive ./dummy
    + path2=/build/nix-test/add/store/n7knm3rma6fckrfwkx2vkiql3d1rgs1i-dummy
    + echo /build/nix-test/add/store/n7knm3rma6fckrfwkx2vkiql3d1rgs1i-dummy
    /build/nix-test/add/store/n7knm3rma6fckrfwkx2vkiql3d1rgs1i-dummy
    + test /build/nix-test/add/store/n7knm3rma6fckrfwkx2vkiql3d1rgs1i-dummy '!=' /build/nix-test/add/store/n7knm3rma6fckrfwkx2vkiql3d1rgs1i-dummy
    ++ nix-store --add-fixed sha256 ./dummy
    + path3=/build/nix-test/add/store/1b0chpd74drqysiwsskw53zwlg18rcjl-dummy
    + echo /build/nix-test/add/store/1b0chpd74drqysiwsskw53zwlg18rcjl-dummy
    /build/nix-test/add/store/1b0chpd74drqysiwsskw53zwlg18rcjl-dummy
    + test /build/nix-test/add/store/n7knm3rma6fckrfwkx2vkiql3d1rgs1i-dummy '!=' /build/nix-test/add/store/1b0chpd74drqysiwsskw53zwlg18rcjl-dummy
    ++ nix-store --add-fixed sha1 --recursive ./dummy
    + path4=/build/nix-test/add/store/dgvdyhjxyg2ychmw8nm1vb0cdszqqnyr-dummy
    + echo /build/nix-test/add/store/dgvdyhjxyg2ychmw8nm1vb0cdszqqnyr-dummy
    /build/nix-test/add/store/dgvdyhjxyg2ychmw8nm1vb0cdszqqnyr-dummy
    + test /build/nix-test/add/store/n7knm3rma6fckrfwkx2vkiql3d1rgs1i-dummy '!=' /build/nix-test/add/store/dgvdyhjxyg2ychmw8nm1vb0cdszqqnyr-dummy
    ++ nix-store -q --hash /build/nix-test/add/store/n7knm3rma6fckrfwkx2vkiql3d1rgs1i-dummy
    + hash1=sha256:0lc8c8k1yc8m563wxg9ikalz4q9f56gc667qnnsjiwgiv7ya8xbw
    + echo sha256:0lc8c8k1yc8m563wxg9ikalz4q9f56gc667qnnsjiwgiv7ya8xbw
    sha256:0lc8c8k1yc8m563wxg9ikalz4q9f56gc667qnnsjiwgiv7ya8xbw
    ++ nix-hash --type sha256 --base32 ./dummy
    + hash2=0ip26j2h11n1kgkz36rl4akv694yz65hr72q4kv4b3lxcbi65b3p
    + echo 0ip26j2h11n1kgkz36rl4akv694yz65hr72q4kv4b3lxcbi65b3p
    0ip26j2h11n1kgkz36rl4akv694yz65hr72q4kv4b3lxcbi65b3p
    + test sha256:0lc8c8k1yc8m563wxg9ikalz4q9f56gc667qnnsjiwgiv7ya8xbw = sha256:0ip26j2h11n1kgkz36rl4akv694yz65hr72q4kv4b3lxcbi65b3p
make: *** [mk/lib.mk:128: tests/add.sh.test] Error 1
make: *** Waiting for unfinished jobs....

i've seen this failure before and filed a bug: #294433

full log: https://gist.githubusercontent.com/GrahamcOfBorg/a54aaa359754668dc899a98c49f6db7f/raw/6d8ed13eda6981f76fcffded3cadc6ff43f06795/ofborg-eval-lib-tests

re-runing the eval usually clears it:
@ofborg eval

@ghost ghost mentioned this pull request Mar 10, 2024
Closed
@ShamrockLee
Copy link
Contributor Author

ShamrockLee commented Mar 10, 2024 •
edited

I'm running nix-build --arg pkgs import ./. {} ./lib/tests/release.nix locally on my laptop. There are chances of failure caused by network issue during source fetching, and this PR changes a bunch of FOD out path, implying a whole lot of re-fetches. It takes multiple re-running to fetch all of them when the network connection is less than ideal.

@ShamrockLee ShamrockLee mentioned this pull request Mar 10, 2024
Merged
@ShamrockLee
Copy link
Contributor Author

Update: Manual run on my x86_64-linux NixOS laptop succeeded.

Command: nix-build --arg pkgs "import ./. {}" ./lib/tests/release.nix --no-out-link

@ShamrockLee
Copy link
Contributor Author

@ofborg eval

@ShamrockLee
fetchgit: takes passthru argument
49b921b
@ShamrockLee
fetchFromGitHub: take passthru and enhance overriding
165ecf4 
Take optional argument `passthru` for custom passthru attribute set.

Update the `meta` attribute via `<pkg>.overrideAttrs` instead of
attribute set update (`//`).

Attach attributes `owner`, `repo` and `rev` via `passthru` instead of
attribute set update.
@ShamrockLee
fetchFromGitHub: force re-fetch when rev changes
b6f434c 
Prefix the default value of `name` with `rev` to force re-fetch
everytime `rev` changes.
@ShamrockLee
Copy link
Contributor Author

Bump as #294334 is merged and has headed staging.

@ShamrockLee
Copy link
Contributor Author

@ofborg eval

@eclairevoyant eclairevoyant mentioned this pull request Mar 20, 2024
Open
@lheckemann
Copy link
Member

Similar changes were rejected previously: #59858 (review)

@ShamrockLee
Copy link
Contributor Author

ShamrockLee commented Mar 23, 2024 •
edited

Similar changes were rejected previously: #59858 (review)

@lheckemann Thanks a lot for this information!

Things have changed a lot this five years. We don't even have stdenv.mkDerivation and <pkg>.overrideAttrs taking finalAttrs back then. As Nixpkgs Manual explicitly specifies that sourceRoot must be formed using ${src.name} for fetchgit-based src and deprecates the src.name == "source" assumption, the rejection is no longer valid.

@eclairevoyant
Copy link
Member

eclairevoyant commented Mar 23, 2024 •
edited

Actually the concern we have now was the same concern from 7 years ago 馃檭

At least for me descriptive names in /nix/store are important (for example if I want to look at the sources of a package that I've recently built I just ls -d /nix/store/*name*), and independence of the hash from the fetch method is not

And now every nix user gets to trip on FOD moments, so badly that we have to warn them in the manual about it.

Anyway maybe the new RFC will help sort it.

@ShamrockLee ShamrockLee mentioned this pull request Mar 23, 2024
Open
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
2.status: merge conflict 6.topic: fetch 6.topic: nixos 8.has: changelog 8.has: documentation 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-darwin-stdenv 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ShamrockLee @lheckemann @eclairevoyant @wegank