fetchFromGitHub: use passthru instead of // to expose rev and friends; fetchgit: support adding extra passthru values#370432
fetchFromGitHub: use passthru instead of // to expose rev and friends; fetchgit: support adding extra passthru values#370432TomaSajt wants to merge 2 commits intoNixOS:masterfrom
Conversation
a4dab12 to
34a6819
Compare
philiptaron
left a comment
There was a problem hiding this comment.
I really like this direction, and I'd love to see the other fetchers brought into line. I have a few questions about order of application and expectations from these derivation factory functions. They're more curious than hard opinions.
| gitRepoUrl = url; | ||
| inherit tag; | ||
| }; | ||
| } // passthru; |
There was a problem hiding this comment.
I really like how this allows the caller to provide values for passthru. I don't like how the caller can override gitRepoUrl and tag, since I can't really come up with good usecases for those that aren't just the user shooting themselves in the foot. What do you think?
The other way would let the user provide whatever, but would assure users that gitRepoUrl and tag would be canonical from this function... so this:
passthru = passthru // {
gitRepoUrl = url;
inherit tag;
};
I could also see rev (aka revWithTag) being on this list for similar reasons.
There was a problem hiding this comment.
My personal opinion is this: if the consumer sets a value, it should be set or respected in some way (e.g. override default value, append to a default list, etc.). A NOOP is much more surprising than a broken package.
There was a problem hiding this comment.
Yes, I also don't like NOOP. Maybe I'm trying to construct a static language out of a dynamic one, but I do like this:
nixpkgs/pkgs/build-support/replace-vars/replace-vars-with.nix
Lines 75 to 79 in 5e4cd3d
The idea being that the parameter is there to supplement the set of values in passthru but not override values which are expected to have a definite meaning computed in the function.
There was a problem hiding this comment.
gitRepoUrl is IIRC used in nix-update, but i believe you should be allowed to clobber it if you really want to.
| inherit tag rev deepClone fetchSubmodules sparseCheckout fetchLFS; url = gitRepoUrl; | ||
| inherit tag rev deepClone fetchSubmodules sparseCheckout fetchLFS; | ||
| url = gitRepoUrl; | ||
| passthru = { inherit owner repo; } // passthru; |
There was a problem hiding this comment.
Same update ordering question here.
| passthru = { | ||
| inherit gitRepoUrl; | ||
| }; | ||
| passthru = { inherit owner repo tag rev gitRepoUrl; } // passthru; |
|
Also note that even if this fixes calling Sidenote: I really wish the main attrset of |
|
This does some of what I drafted as patch#2 in Mic92/nix-update#281 and forgot about 👍 One property I'd like for this PR to achieve is for |
We could do that, though I'm not 100% it's for the best. What's interesting too is the fact that we could use (I'm not sure if you'd ever want to |
|
Ah, i didn't consider that. Your approach is sound! |
|
also see #294329 |
|
Superseded by #456751 |
Closes #370418
Other than the main change about passing some values with
passthruinstead of//there are some minor extra changes which should not cause any issues:passthruAttrstoextraFetcherAttrsto not have two meanings of passthru in the same file.inherit nameis now a bit earlier in the construction of the combined attrset.Since we don't filter
nameout from@attrsthe// extraFetcherAttrswill automatically include thenamevalue if it is passed to the function.This means the only thing
inherit name;does is that it sets the default value.I could replace
inherit name;withname = "source";and everything would be the same.name ? "source"from the function argsNote: I did not update
fetchFromGitLaband other fetchers.Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.