-
Notifications
You must be signed in to change notification settings - Fork 3
fetchFromGitHub: force re-fetch when rev changes #11
fetchFromGitHub: force re-fetch when rev changes #11
Conversation
Is this NixOS/nixpkgs@c3255fe8ec32#commitcomment-25289921, not a concern which lead the original PR maker to create NixOS/nixpkgs#294329 |
This is true and those two commits are already included in the first PR as well as the present PR. This change does not by itself invalidate all hashes. I checked the |
b48ebc0
to
8c0a99d
Compare
Removing the fix hash commit to avoid conflicts |
{ owner, repo, rev, name ? "source" | ||
{ owner, repo, rev | ||
, name ? null # Override with nullbto use the default value | ||
, pname ? "source-${owner}-${repo}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH, I'd also change source
to something like github-repo
, just to make it clearer in the name that the derivation comes from a GitHub checkout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a slightly different proposal: append the github base url as provenance like so: /nix/store/wzvnh3p7gv1qlqhb3iif878bfffzrvaz-source-pypa-build-refs-tags-1.1.1-github.com.drv
. That's a good self-description, leaves the source
that people might be used to, and generalizes unambiguously to other fetchFrom
s. We can also put it before the owner
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only potential issue I see is creating some pretty long path names, but I don't see it being a massive issue. Seems fine to me.
Prefix the default value of `name` with `rev` to force re-fetch everytime `rev` changes.
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.
8c0a99d
to
7494c65
Compare
{ owner, repo, rev, name ? "source" | ||
{ owner, repo, rev | ||
, name ? null # Override with null to use the default value | ||
, pname ? "source-${githubBase}-${owner}-${repo}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/nix/store/f291banfgj2vjz1qsvmpm09awn2gl4pj-source-github.com-pypa-build-refs-tags-1.1.1.drv
wdyt @Sorixelle ?
Since we have yet to hit the ground running, we have a golden opportunity to fix some of those long-standing issues in upstream that don't get fixed to avoid disturbing the whole project. Specifically, this is the fact that
fetchFromGitHub
currently outputs a FOD with a very generic name. As a result, if a maintainer changes a version number but forgets about the output hash, it may not get caught. Error messages are also less useful. If we can fix that we will have much better DX moving forward.To illustrate: I tried to rebuild
nixdoc
to see if it was working and I immediately caught a stale hash.Based on NixOS/nixpkgs#294068