fetchgit{,hub}: add tag argument #355973
fetchgit{,hub}: add tag argument #355973
Conversation
Atemu
added
backport release-24.05
backport release-24.11
github-actions
bot
added
the
8.has: documentation
It's become a common pattern to use `rev = "refs/tags/${version}"` rather than
just `rev = version` to ensure that the tag gets fetched rather than a branch
that has the same name. This has so far been done using boilerplate though, so
let's add a simple abstraction to fetch a tag instead.
|
#355685 is now in master. |
Atemu
added
9.needs: port to stable
|
This will not apply to 24.05 cleanly because of the docs but I will backport the functionality if no breakage is detected on unstable after this is merged. There really shouldn't be any but just to be sure. |
There was a problem hiding this comment.
I really like this direction, but the implementation (done with defaults) means it's possible for a user to specify both a tag and a rev, and the derivation will silently pick the rev.
Instead, how about implementing this with a let block, so that the derivation-creating function is in charge of exactly how and in what circumstances rev and tag interact?
|
I feel like that is a contrived example, do you expect that to actually happen? This seems to be a theoretical concern to me, not a practical one. |
wegank
added
the
12.approvals: 1
|
I think I'd like to go ahead with this and self-merge unless someone objects within the next week or so. |
|
What about |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/fetchgit-hub-add-tag-argument/56710/6 |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/how-to-properly-package-a-executable-file/57184/25 |
|
I wasn't too clear on this: please feel absolutely free to port tag support to other VCS fetechers yourself. I likely won't be the one to do it. This would also be a good first PR as it that's not very coplicated to do or verify. |
|
One nit: after setting the |
|
This idea is so good, we should have had it earlier 👍🏼 🥰 |
And if anyone does decide to take this on, please add me as a reviewer! I'd love to help you get it to merged. 🖥️ 🚀 |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/community-team-updates/56458/11 |
|
Exposed an issue with using |
|
It seems like it would be best for this to expose the appropriate |
|
We could do that (PRs welcome) but you really shouldn't be depending on |
|
I think |
You couldn't use |
|
Sure you could; https://github.com/NixOS/nixpkgs/blob/refs/tags/24.11-pre/README.md works fine and is fact better than the alternative for precisely the same reason we want to use |
It's become a common pattern to use
rev = "refs/tags/${version}"rather than justrev = versionto ensure that the tag gets fetched rather than a branch that has the same name. This is done using boilerplate though, so let's add a simple abstraction for fetching tags instead.The first two commits are from #355685 which should be merged first. Please review those separately.This should be backported as it's a non-breaking addition and because people will likely want to make use of this in changes which they want to backport.
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.