Skip to content

fetchFromGitHub: takes passthru and enhance overriding#294329

Closed
ShamrockLee wants to merge 2 commits intoNixOS:masterfrom
ShamrockLee:fetchgithub-passthru
Closed

fetchFromGitHub: takes passthru and enhance overriding#294329
ShamrockLee wants to merge 2 commits intoNixOS:masterfrom
ShamrockLee:fetchgithub-passthru

Conversation

@ShamrockLee
Copy link
Contributor

@ShamrockLee ShamrockLee commented Mar 8, 2024

Description of changes

This PR

  • Make fetchers fetchgit fetchFromGitHub support passthru as an optional argument. This enables user to specify passthru arguments the idiomatic, non-hacky way. This also make their interface more aligned to that of fetchurl.
  • Enhance overriding for result FOD of fetchFromGitHub with <pkg>.overrideAttrs and passthru.
    • Update the meta attribute of the result package by <pkgs>.overrideAttrs the one returned by the base fetcher.
    • Attach owner, repo and rev attributes via passthru instead of directly updating the attribute set via //, preserving these attributes across <pkg>.overrideAttrs.

Things done

  • Built on platform(s)
    • x86_64-linux (no rebuild)
    • 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.

@github-actions github-actions bot added the 6.topic: fetch Fetchers (e.g. fetchgit, fetchsvn, ...) label Mar 8, 2024
@ShamrockLee ShamrockLee mentioned this pull request Mar 8, 2024
Draft
13 tasks
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Mar 8, 2024
@nixos-discourse
Copy link

nixos-discourse commented Mar 10, 2024

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

https://discourse.nixos.org/t/prs-ready-for-review/3032/3612

AndersonTorres
AndersonTorres reviewed Mar 10, 2024
View reviewed changes
@ShamrockLee ShamrockLee force-pushed the fetchgithub-passthru branch from 66bf99f to 2a28536 Compare March 10, 2024 17:32
SuperSandro2000
SuperSandro2000 reviewed Mar 11, 2024
View reviewed changes
@ShamrockLee ShamrockLee force-pushed the fetchgithub-passthru branch 2 times, most recently from 82514db to 3be9877 Compare March 11, 2024 17:05
@wegank wegank added the 12.approvals: 2 This PR was reviewed and approved by two persons. label Mar 19, 2024
@ShamrockLee
Copy link
Contributor Author

ShamrockLee commented Mar 23, 2024

Since it has two approvals and passes all CI tests, could we merge this PR?

#294068 will be ready for review as soon as this PR is merged and goes from master into staging.

@nixos-discourse
Copy link

nixos-discourse commented Apr 28, 2024

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

https://discourse.nixos.org/t/prs-ready-for-review/3032/3842

@nixos-discourse
Copy link

nixos-discourse commented Apr 28, 2024

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

https://discourse.nixos.org/t/prs-already-reviewed/2617/1608

@isabelroses isabelroses mentioned this pull request May 14, 2024
Merged
@nixos-discourse
Copy link

nixos-discourse commented May 20, 2024

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

https://discourse.nixos.org/t/prs-already-reviewed/2617/1660

SuperSandro2000
SuperSandro2000 reviewed Jun 2, 2024
View reviewed changes
@ShamrockLee ShamrockLee force-pushed the fetchgithub-passthru branch from 3be9877 to 77deb59 Compare June 2, 2024 17:46
@ShamrockLee ShamrockLee force-pushed the fetchgithub-passthru branch from 77deb59 to 138a39d Compare June 2, 2024 23:08
@ShamrockLee
Copy link
Contributor Author

ShamrockLee commented Jun 2, 2024

I just changed how we improve the meta specification from <pkg>.overrideAttrs to passing it down to the base fetcher.

fetchFromGitHub specifies its custom meta.position. Given that meta.position is currently not <pkg>.overrideAttrs-overridarble (#295907), it needs to be specified during the initial attribute specification of stdenv.mkDerivation.

Passing it down to the base fetcher also simplifies the implementation.

@ShamrockLee
Copy link
Contributor Author

ShamrockLee commented Jun 17, 2024

@SuperSandro2000 Could you help take another look?

@wegank wegank removed the 12.approvals: 2 This PR was reviewed and approved by two persons. label Jun 26, 2024
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 9, 2024
@nix-owners nix-owners bot requested a review from philiptaron December 30, 2024 18:44
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Dec 31, 2024
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 4, 2025
@ShamrockLee
fetchgit: takes passthru argument
a985714
@ShamrockLee
fetchFromGitHub: take passthru and enhance overriding
4aff924 
Take optional argument `passthru` for custom passthru attribute set.

Pass the `meta` attribute down to the base builder instead of
attaching to the derivation via attribute set updating (`//`).

Attach attributes `owner`, `repo`, `rev` and `tags` via `passthru`
instead of attribute set update.
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 15, 2025
@pbsds
Copy link
Member

pbsds commented Jan 19, 2025

parallel effort: #370432

I've yet to read through either PR in full

@wegank wegank added the 12.approvals: 2 This PR was reviewed and approved by two persons. label Jan 19, 2025
@Atemu Atemu removed their request for review January 19, 2025 15:50
@SuperSandro2000 SuperSandro2000 mentioned this pull request Jan 21, 2025
Closed
13 tasks
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 2, 2025
@pbsds pbsds mentioned this pull request Jun 4, 2025
Open
@philiptaron
Copy link
Contributor

philiptaron commented Jun 4, 2025

@ShamrockLee, would this now want to use lib.extendMkDerivation now?

@ShamrockLee
Copy link
Contributor Author

ShamrockLee commented Jun 9, 2025 •
edited
Loading

@ShamrockLee, would this now want to use lib.extendMkDerivation now?

fetchFromGitHub chooses to base itself on either fetchzip or fetchgit based on arguments it takes, which is logically impossible to fit into lib.extendMkDreivation or override by overrideAttrs.

@eclairevoyant eclairevoyant mentioned this pull request Sep 21, 2025
Open
@adeci adeci mentioned this pull request Oct 26, 2025
Closed
13 tasks
@ShamrockLee
Copy link
Contributor Author

ShamrockLee commented Oct 27, 2025 •
edited
Loading

@philiptaron It turns out to be possible to decide on the base constructor dynamically, given that they are based on the same ancestor constructor. The glitch is that it can only be decided once, i.e., <pkg>.overrideAttrs cannot change the decision.

Here's the reasoning:
#455637 (comment)

BTW, we need to fix the hideous owner and repo passing and some more stuff before fetchFromGitHub can be reconstructed with lib.extendMkDerivation.

@ShamrockLee ShamrockLee mentioned this pull request Oct 29, 2025
Merged
13 tasks
@ShamrockLee
Copy link
Contributor Author

ShamrockLee commented Nov 27, 2025

Surpassed by:

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

Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 approved these changes

@philiptaron philiptaron Awaiting requested review from philiptaron

@pbsds pbsds Awaiting requested review from pbsds

+1 more reviewer

@AndersonTorres AndersonTorres AndersonTorres approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: fetch Fetchers (e.g. fetchgit, fetchsvn, ...) 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. 12.approvals: 2 This PR was reviewed and approved by two persons.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@ShamrockLee @nixos-discourse @pbsds @philiptaron @AndersonTorres @SuperSandro2000 @wegank