Merge legacy fetchGit-builtin with the generic fetchTree-function
#3549
Conversation
|
@domenkozar would you mind taking a look at this? :) |
|
The reason for not adding a |
I see. When I filed this patch, I wasn't' sure whether it's intended that This would still require some modifications in the |
|
Yes, please. All the other changes (like returning 102) look good to me. |
The original idea was to implement a git-fetcher in Nix's core that supports content hashes[1]. In NixOS#3549[2] it has been suggested to actually use `fetchTree` for this since it's a fairly generic wrapper over the new fetcher-API[3] and already supports content-hashes. This patch implements a new git-fetcher based on `fetchTree` by incorporating the following changes: * Removed the original `fetchGit`-implementation and replaced it with an alias on the `fetchTree` implementation. * Ensured that the `git`-fetcher from `libfetchers` always computes a content-hash and returns an "empty" revision on dirty trees (the latter one is needed to retain backwards-compatibility). * The hash-mismatch error in the fetcher-API exits with code 102 as it usually happens whenever a hash-mismatch is detected by Nix. * Removed the `flakes`-feature-flag: I didn't see a reason why this API is so tightly coupled to the flakes-API and at least `fetchGit` should remain usable without any feature-flags. * It's only possible to specify a `narHash` for a `git`-tree if either a `ref` or a `rev` is given[4]. * It's now possible to specify an URL without a protocol. If it's missing, `file://` is automatically added as it was the case in the original `fetchGit`-implementation. [1] NixOS#3216 [2] NixOS#3549 (comment) [3] NixOS#3459 [4] NixOS#3216 (comment)
Ma27
changed the title
narHash in builtins.fetchGitfetchGit-builtin with the generic fetchTree-function
|
Rebased onto latest master and merged @globin since you were quite interested in this and I had to change a lot, you may want to take another look at this :) |
|
ping @domenkozar @edolstra |
|
I'll leave it up to Eelco for final review. |
|
Would you mind rebasing this on the flakes branch? I made some changes to the fetcher API there. (In particular |
|
Yes, already on it :) |
The original idea was to implement a git-fetcher in Nix's core that supports content hashes[1]. In NixOS#3549[2] it has been suggested to actually use `fetchTree` for this since it's a fairly generic wrapper over the new fetcher-API[3] and already supports content-hashes. This patch implements a new git-fetcher based on `fetchTree` by incorporating the following changes: * Removed the original `fetchGit`-implementation and replaced it with an alias on the `fetchTree` implementation. * Ensured that the `git`-fetcher from `libfetchers` always computes a content-hash and returns an "empty" revision on dirty trees (the latter one is needed to retain backwards-compatibility). * The hash-mismatch error in the fetcher-API exits with code 102 as it usually happens whenever a hash-mismatch is detected by Nix. * Removed the `flakes`-feature-flag: I didn't see a reason why this API is so tightly coupled to the flakes-API and at least `fetchGit` should remain usable without any feature-flags. * It's only possible to specify a `narHash` for a `git`-tree if either a `ref` or a `rev` is given[4]. * It's now possible to specify an URL without a protocol. If it's missing, `file://` is automatically added as it was the case in the original `fetchGit`-implementation. [1] NixOS#3216 [2] NixOS#3549 (comment) [3] NixOS#3459 [4] NixOS#3216 (comment)
|
@edolstra rebased onto |
|
@edolstra as this still applies properly to |
The original idea was to implement a git-fetcher in Nix's core that supports content hashes[1]. In NixOS#3549[2] it has been suggested to actually use `fetchTree` for this since it's a fairly generic wrapper over the new fetcher-API[3] and already supports content-hashes. This patch implements a new git-fetcher based on `fetchTree` by incorporating the following changes: * Removed the original `fetchGit`-implementation and replaced it with an alias on the `fetchTree` implementation. * Ensured that the `git`-fetcher from `libfetchers` always computes a content-hash and returns an "empty" revision on dirty trees (the latter one is needed to retain backwards-compatibility). * The hash-mismatch error in the fetcher-API exits with code 102 as it usually happens whenever a hash-mismatch is detected by Nix. * Removed the `flakes`-feature-flag: I didn't see a reason why this API is so tightly coupled to the flakes-API and at least `fetchGit` should remain usable without any feature-flags. * It's only possible to specify a `narHash` for a `git`-tree if either a `ref` or a `rev` is given[4]. * It's now possible to specify an URL without a protocol. If it's missing, `file://` is automatically added as it was the case in the original `fetchGit`-implementation. [1] NixOS#3216 [2] NixOS#3549 (comment) [3] NixOS#3459 [4] NixOS#3216 (comment)
|
@edolstra rebased onto latest |
|
@edolstra Is there a rough ETA for merging the flakes branch? This change would be desirable for us too: if flakes will be merged soon that's fine, but if not, then maybe we could back-port something like the earlier |
|
@Ericson2314 I can merge it after the RFC is merged. |
|
@edolstra may I ask why though? Those fetchers are even on non-flakes master atm. |
The original idea was to implement a git-fetcher in Nix's core that supports content hashes[1]. In NixOS#3549[2] it has been suggested to actually use `fetchTree` for this since it's a fairly generic wrapper over the new fetcher-API[3] and already supports content-hashes. This patch implements a new git-fetcher based on `fetchTree` by incorporating the following changes: * Removed the original `fetchGit`-implementation and replaced it with an alias on the `fetchTree` implementation. * Ensured that the `git`-fetcher from `libfetchers` always computes a content-hash and returns an "empty" revision on dirty trees (the latter one is needed to retain backwards-compatibility). * The hash-mismatch error in the fetcher-API exits with code 102 as it usually happens whenever a hash-mismatch is detected by Nix. * Removed the `flakes`-feature-flag: I didn't see a reason why this API is so tightly coupled to the flakes-API and at least `fetchGit` should remain usable without any feature-flags. * It's only possible to specify a `narHash` for a `git`-tree if either a `ref` or a `rev` is given[4]. * It's now possible to specify an URL without a protocol. If it's missing, `file://` is automatically added as it was the case in the original `fetchGit`-implementation. [1] NixOS#3216 [2] NixOS#3549 (comment) [3] NixOS#3459 [4] NixOS#3216 (comment)
|
@edolstra rebased conflicts against master. Is there anything else tbd to get this merged? |
|
@edolstra the RFC is closed now - anything else we'd have to wait for until you can merge this? :) |
The original idea was to implement a git-fetcher in Nix's core that supports content hashes[1]. In NixOS#3549[2] it has been suggested to actually use `fetchTree` for this since it's a fairly generic wrapper over the new fetcher-API[3] and already supports content-hashes. This patch implements a new git-fetcher based on `fetchTree` by incorporating the following changes: * Removed the original `fetchGit`-implementation and replaced it with an alias on the `fetchTree` implementation. * Ensured that the `git`-fetcher from `libfetchers` always computes a content-hash and returns an "empty" revision on dirty trees (the latter one is needed to retain backwards-compatibility). * The hash-mismatch error in the fetcher-API exits with code 102 as it usually happens whenever a hash-mismatch is detected by Nix. * Removed the `flakes`-feature-flag: I didn't see a reason why this API is so tightly coupled to the flakes-API and at least `fetchGit` should remain usable without any feature-flags. * It's only possible to specify a `narHash` for a `git`-tree if either a `ref` or a `rev` is given[4]. * It's now possible to specify an URL without a protocol. If it's missing, `file://` is automatically added as it was the case in the original `fetchGit`-implementation. [1] NixOS#3216 [2] NixOS#3549 (comment) [3] NixOS#3459 [4] NixOS#3216 (comment)
The original idea was to implement a git-fetcher in Nix's core that supports content hashes[1]. In NixOS#3549[2] it has been suggested to actually use `fetchTree` for this since it's a fairly generic wrapper over the new fetcher-API[3] and already supports content-hashes. This patch implements a new git-fetcher based on `fetchTree` by incorporating the following changes: * Removed the original `fetchGit`-implementation and replaced it with an alias on the `fetchTree` implementation. * Ensured that the `git`-fetcher from `libfetchers` always computes a content-hash and returns an "empty" revision on dirty trees (the latter one is needed to retain backwards-compatibility). * The hash-mismatch error in the fetcher-API exits with code 102 as it usually happens whenever a hash-mismatch is detected by Nix. * Removed the `flakes`-feature-flag: I didn't see a reason why this API is so tightly coupled to the flakes-API and at least `fetchGit` should remain usable without any feature-flags. * It's only possible to specify a `narHash` for a `git`-tree if either a `ref` or a `rev` is given[4]. * It's now possible to specify an URL without a protocol. If it's missing, `file://` is automatically added as it was the case in the original `fetchGit`-implementation. [1] NixOS#3216 [2] NixOS#3549 (comment) [3] NixOS#3459 [4] NixOS#3216 (comment)
|
@edolstra resolved the merge conflict. Is there anything to discuss about the pending two comments you've left? |
tests/fetchGit.sh
Outdated
| @@ -123,7 +129,7 @@ path2=$(nix eval --impure --raw --expr "(builtins.fetchGit file://$repo).outPath | |||
| # Using local path with branch other than 'master' should work when clean or dirty | |||
| path3=$(nix eval --impure --raw --expr "(builtins.fetchGit $repo).outPath") | |||
| # (check dirty-tree handling was used) | |||
| [[ $(nix eval --impure --raw --expr "(builtins.fetchGit $repo).rev") = 0000000000000000000000000000000000000000 ]] | |||
| [[ $(nix eval --impure --expr "(builtins.fetchGit $repo).rev or null") = null ]] | |||
There was a problem hiding this comment.
This breaks compatibility since there might be projects that expect that rev is always set. fetchGit (but not fetchTree) on dirty trees should continue to return 000....
There was a problem hiding this comment.
so, partly revert my change to ensure fetchGit returns an empty rev here?
There was a problem hiding this comment.
Fixed. However I'm well-aware that my current fix is fairly ugly since I didn't manage to allocate a new attr to v in prim_fetchGit. If you have a better solution, please tell me :)
If a repo is dirty, it used to return a `rev` object with an "empty"
sha1 (0000000000000000000000000000000000000000). Please note that this
only applies for `builtins.fetchGit` and *not* for `builtins.fetchTree{
type = "git"; }`.
|
@Ma27 Merged, thanks for all your work on this! |
|
You're welcome :) I'm surprised though that there's now a failingtest although I ran |
|
For the macos issue I asked in |
The original idea was to implement a git-fetcher in Nix's core that
supports content hashes[1]. In #3549[2] it has been suggested to
actually use
fetchTreefor this since it's a fairly generic wrapperover the new fetcher-API[3] and already supports content-hashes.
This patch implements a new git-fetcher based on
fetchTreebyincorporating the following changes:
Removed the original
fetchGit-implementation and replaced it with analias on the
fetchTreeimplementation.Ensured that the
git-fetcher fromlibfetchersalways computes acontent-hash and returns an "empty" revision on dirty trees (the
latter one is needed to retain backwards-compatibility).
The hash-mismatch error in the fetcher-API exits with code 102 as it
usually happens whenever a hash-mismatch is detected by Nix.
Removed the
flakes-feature-flag: I didn't see a reason why this APIis so tightly coupled to the flakes-API and at least
fetchGitshouldremain usable without any feature-flags.
It's only possible to specify a
narHashfor agit-tree if either arefor arevis given[4].It's now possible to specify an URL without a protocol. If it's missing,
file://is automatically added as it was the case in the originalfetchGit-implementation.[1] #3216
[2] #3549 (comment)
[3] #3459
[4] #3216 (comment)
cc @edolstra @globin @xbreak @FRidh