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

fetchFromGitHub: make owner and repo accessible #42771

Closed
wants to merge 1 commit into from

Conversation

ryantm
Copy link
Member

@ryantm ryantm commented Jun 29, 2018

I would like to be able to get the repo and owner attrs of a fetchFromGitHub derivation so that I can add changelog information to nixpkgs-update's pull requests.

I renamed passthruArgs because I thought this is a confusing name given we have a passthru concept already, and added owner, repo, and rev to the passthru attrs for the fetchzip and fetchgit derivations. I also removed "inherit rev" from the attrset, since it seems to be covered by the passthru attrs.

I'm not that familiar with this kind of nix editing and this is an important derivation, so please check my work!

@ryantm
Copy link
Member Author

ryantm commented Jul 3, 2018

Can we also merge this into staging? nixpkgs-update uses both master and staging.

@@ -257,8 +257,8 @@ with pkgs;
fetcherArgs = (if fetchSubmodules
then { inherit rev fetchSubmodules; url = "${baseUrl}.git"; }
else ({ url = "${baseUrl}/archive/${rev}.tar.gz"; } // privateAttrs)
) // passthruAttrs // { inherit name; };
in fetcher fetcherArgs // { meta.homepage = baseUrl; inherit rev; };
Copy link
Member

Choose a reason for hiding this comment

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

Actually, wouldn't it work to just use inherit owner repo rev; here?

Copy link
Member

@infinisil infinisil Jul 3, 2018

Choose a reason for hiding this comment

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

I'll test this. Then probably not a single derivation will even change, because just overriding the derivation attribute sets attributes doesn't influence the drv, which is probably not the case with passthru.

Copy link
Member

Choose a reason for hiding this comment

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

Turns out neither of those approaches influence the derivation, so I think passthru is the cleaner way to do this then. passthru is exactly meant for this kind of stuff.

Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

This is looking good to me then

@@ -257,8 +257,8 @@ with pkgs;
fetcherArgs = (if fetchSubmodules
then { inherit rev fetchSubmodules; url = "${baseUrl}.git"; }
else ({ url = "${baseUrl}/archive/${rev}.tar.gz"; } // privateAttrs)
) // passthruAttrs // { inherit name; };
in fetcher fetcherArgs // { meta.homepage = baseUrl; inherit rev; };
Copy link
Member

Choose a reason for hiding this comment

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

Turns out neither of those approaches influence the derivation, so I think passthru is the cleaner way to do this then. passthru is exactly meant for this kind of stuff.

@orivej
Copy link
Contributor

orivej commented Jul 3, 2018

This looks incomplete: you don't pass through the information that the owner and repo are on github.com. How would you know that? If the answer is by looking at the URL, then you could split owner, repo and rev from the URL as well.

@ryantm
Copy link
Member Author

ryantm commented Jul 4, 2018

@orivej Thanks, that's a good point! You're right that I don't need this change.

@ryantm ryantm closed this Jul 4, 2018
@infinisil
Copy link
Member

It would be possible to use passthru.github = { inherit owner repo; }. I don't know about you guys, but I try to avoid parsing string in Nix however I can!

@orivej
Copy link
Contributor

orivej commented Jul 4, 2018

@infinisil, @ryantm probably parses it another language. This PR is well intentioned and almost complete, but the exact set of attributes that should be passed through is not obvious (should there be passthru.github.owner = "";, or passthru = { type = "github"; owner = ""; };?), and if @ryantm does not really need it, then we don't have to decide this.

@orivej
Copy link
Contributor

orivej commented Jul 4, 2018

One benefit of parsing URLs is that this supports derivations that use fetchzip and fetchgit directly to fetch from GitHub.

@infinisil
Copy link
Member

Good points

@ryantm ryantm deleted the fetchfromgithub branch July 4, 2018 03:14
@ryantm
Copy link
Member Author

ryantm commented Jul 4, 2018

@ryantm probably parses it another language.

Yes, Haskell. Here is my fake parser:
nix-community/nixpkgs-update@07aaa9f#diff-6b5e4761bf4e8e9e9a3eaac708f0adb9R29

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

Successfully merging this pull request may close these issues.

None yet

4 participants