fetchers/git: make relative path absolute for local repo #12107

bryango · 2024-12-26T12:45:52Z

Motivation

Closes #9708 (again).

This patch:

(naively) Resolves relative paths to local git repos, returning absolute paths, thus allowing flake URIs such as git+file:./${submodule} to continue working.
Adds tests to prevent future breakages.

Context

Flake URIs such as git+file:./${submodule} used to work before 2.18, but broke in 2.19, was fixed later, and broke again recently since 3e0129c, resulting in a core-dumped assertion error in #9708 (comment).

This PR suppresses the error and allows git+file:./${submodule}to continue working. However, this fix is not ideal and should potentially be superseded by a better submodule implementation in the future. See:

A better fix is outlined by @roberth in https://discourse.nixos.org/t/57783, namely:

This bug seems to have allowed relative git flake inputs to be resolved against the current working directory (as in POSIX), and this tends to work out ok in the context of flakes, but is the wrong behavior, as it should resolve against the flake.nix base directory instead.
Supporting relative paths in fetchTree would add significant value.
This behavior was used as a workaround for lacking relative flake input or “subflake” support.

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

Friendly pings:

@roberth and @tomberek for reviews
@DamienCassou and @aij for testing

aij

I gave it a quick try and it does work for me:

ivan@tobati:~/nixops/aij$ /nix/store/bn0djaymav6qa4zg5vaskzcaija5nq2p-nix-2.26.0pre20241227_9111645/bin/nix flake update 
warning: updating lock file '/home/ivan/nixops/aij/flake.lock':
• Updated input 'unstable':
    'git+file:unstable?rev=3f0036916daca9836ba2c3f236b0ae051949a0a8' (2024-12-21)
  → 'git+file:unstable?ref=refs/heads/ipmiutil&rev=174ae1b581ddd6d5a6c55684da392e0baf500b1e' (2024-12-27)

roberth

Partial fix

edolstra · 2024-12-31T15:43:55Z

For the example in #9709, namely

inputs.nixpkgs.url = git+file:./nixpkgs;

doesn't this result in a lock file that has the absolute path of the original Git repo in it? I.e. if I run nix flake lock on such a flake in /home/bob/repo, doesn't it result in a lock file that references /home/bob/repo/nixpkgs?

bryango · 2025-01-01T06:53:42Z

inputs.nixpkgs.url = git+file:./nixpkgs;
doesn't this result in a lock file that has the absolute path of the original Git repo in it? I.e. if I run nix flake lock on such a flake in /home/bob/repo, doesn't it result in a lock file that references /home/bob/repo/nixpkgs?

@edolstra Actually, no! It still gets locked to the relative path and shows up in flake.lock as file:./nixpkgs, which is great! I have pushed another test to cement this behavior.

bryango · 2025-01-01T06:55:57Z

tests/functional/flakes/flake-in-submodule.sh

+cat > "$rootRepo"/flake.nix <<EOF
+{
+    inputs.subRepo.url = "git+file:./submodule";
+    outputs = { ... }: { };
+}
+EOF
+git -C "$rootRepo" add flake.nix
+git -C "$rootRepo" commit -m "Add flake.nix"
+(
+  cd "$rootRepo"
+  # The submodule must be locked to the relative path,
+  # _not_ the absolute path:
+  [[ $(nix flake metadata --json | jq -r .locks.nodes.subRepo.locked.url) = "file:./submodule" ]]
+)


submodule does get locked to the relative path file:./submodule. I don't understand why but hey, it works!

bryango · 2025-01-04T12:27:21Z

Draft-ing and un-draft-ing to try to unlock the new CI...
Update: doesn't seem to work, no idea what I should do now haha

roberth · 2025-01-04T21:28:51Z

Rebased for GHA config update

bryango · 2025-01-09T11:06:09Z

Rebased on current master, conflicts resolved, CI happy! Friendly ping @roberth @edolstra for re-review!

Relative, local git repo used to work (for submodules), but it fails after 3e0129c. This commit adds a test to prevent such failure in the future.

bryango · 2025-01-10T07:12:41Z

Rebased again, conflicts resolved, CI happy.

tests/functional/flakes/flakes.sh

roberth · 2025-01-10T08:58:16Z

Thanks!

@mergify queue

…on of NixOS/nix#9708 I got tired of manually running ``` /nix/store/bn0djaymav6qa4zg5vaskzcaija5nq2p-nix-2.26.0pre20241227_9111645/bin/nix flake update ``` for every update while waiting for the fix to make it upstreaam.

roberth · 2025-01-15T19:12:53Z

@mergify queue

mergify · 2025-01-15T19:13:40Z

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 9c239d4

…2107 fetchers/git: make relative path absolute for local repo (backport #12107)

edolstra · 2025-01-16T15:38:18Z

src/libfetchers/git.cc

+        //
+        // See: https://discourse.nixos.org/t/57783 and #9708
+        //
+        repoInfo.url = repoInfo.isLocal ? std::filesystem::absolute(url.path).string() : url.to_string();


I'm a bit baffled by this. As the comment notes, resolving relative to the cwd is wrong since it could be anything. So why are we doing this?

edolstra · 2025-01-16T15:49:04Z

I fear that this PR enshrines some extremely undesirable semantics, namely that libfetcher now depends on the current working directory. As a result, nix build . might succeed, while nix build /path/to/flake would mysteriously fail, or worse, produce another result if the relative path of the submodule happens to exist in the cwd of the caller. IMHO that's unambiguously a bug.

At the very least, we should print a warning that users should not rely on this behaviour.

roberth · 2025-01-16T19:41:44Z

This was a calculated decision to merge. I believe an alternate result is not possible, and that the wrong behavior would only cause a direct failure ("mysteriously fail"). I've approved it as a workaround, not as a solution.

I've written #12281 to clarify the situation and discuss / track the final solution.

bryango · 2025-01-17T03:23:37Z

Yes, this is not the ideal behavior; it's a band-aid to fix the cryptic core-dumped assertion error as in #9708 (comment). Note that this PR was proposed and reviewed way before #12254. If the latter fix achieves the same purpose, ~~we can simply revert 96bd9ba (but keep the tests as it prevents future failures).~~

Update: without 96bd9ba we would still land on an assertion error, so it would be better to proceed from #12277.

Backport git+file:./ fixes to 2.24 (#12107 + #12277)

github-actions bot added the fetching Networking with the outside (non-Nix) world, input locking label Dec 26, 2024

bryango force-pushed the feature branch from 26c93c3 to e971a3b Compare December 26, 2024 12:50

bryango changed the title ~~git: make relative path absolute for local repo~~ fetchers/git: make relative path absolute for local repo Dec 26, 2024

github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Dec 27, 2024

bryango force-pushed the feature branch from af68774 to 9111645 Compare December 27, 2024 09:35

bryango marked this pull request as ready for review December 27, 2024 13:29

bryango requested a review from edolstra as a code owner December 27, 2024 13:29

bryango mentioned this pull request Dec 27, 2024

Flake input git+file:./${submodule} no longer works #9708

Closed

aij approved these changes Dec 27, 2024

View reviewed changes

roberth approved these changes Dec 30, 2024

View reviewed changes

bryango commented Jan 1, 2025

View reviewed changes

bryango marked this pull request as draft January 4, 2025 12:27

bryango marked this pull request as ready for review January 4, 2025 12:28

roberth force-pushed the feature branch from 4afe904 to 6912549 Compare January 4, 2025 21:28

Mic92 requested a review from roberth January 7, 2025 19:15

bryango marked this pull request as draft January 9, 2025 09:50

bryango force-pushed the feature branch from f3dc52f to aaf0ff6 Compare January 9, 2025 10:00

bryango marked this pull request as ready for review January 9, 2025 11:02

bryango added 3 commits January 10, 2025 11:37

fetchers/git: make path absolute for local repo

96bd9ba

tests/flakes: check git+file:./${submodule} protocol

9d088fa

Relative, local git repo used to work (for submodules), but it fails after 3e0129c. This commit adds a test to prevent such failure in the future.

tests/flake-in-submodule: git+file:./* input

37ac18d

bryango force-pushed the feature branch from aaf0ff6 to 37ac18d Compare January 10, 2025 03:45

roberth approved these changes Jan 10, 2025

View reviewed changes

tests/functional/flakes/flakes.sh Show resolved Hide resolved

Clarify cd call in tests/functional/flakes/flakes.sh

d9a50c0

roberth added backport 2.24-maintenance Automatically creates a PR against the branch backport 2.25-maintenance Automatically creates a PR against the branch labels Jan 10, 2025

NixOS deleted a comment from mergify bot Jan 15, 2025

mergify bot added a commit that referenced this pull request Jan 15, 2025

Merge of #12107

bc2f6b0

mergify bot mentioned this pull request Jan 15, 2025

merge queue: embarking master (61467b7) and #12107 together #12263

Closed

37 tasks

mergify bot merged commit 9c239d4 into NixOS:master Jan 15, 2025
13 checks passed

This was referenced Jan 15, 2025

fetchers/git: make relative path absolute for local repo (backport #12107) #12265

Closed

fetchers/git: make relative path absolute for local repo (backport #12107) #12266

Merged

mergify bot added a commit that referenced this pull request Jan 15, 2025

Merge pull request #12266 from NixOS/mergify/bp/2.25-maintenance/pr-1…

98506a5

…2107 fetchers/git: make relative path absolute for local repo (backport #12107)

edolstra reviewed Jan 16, 2025

View reviewed changes

edolstra mentioned this pull request Jan 16, 2025

nix-functional-tests:flakes / flake-in-submodule failed #12273

Closed

2 tasks

edolstra mentioned this pull request Jan 16, 2025

Warn against the use of relative 'git+file:' flake inputs #12277

Merged

roberth mentioned this pull request Jan 16, 2025

Relative git flake inputs git+file:relative/path #12281

Open

bryango mentioned this pull request Jan 17, 2025

Backport git+file:./ fixes to 2.24 (#12107 + #12277) #12287

Merged

roberth added automatic backport This PR is a backport produced by automation (does not trigger backporting) and removed automatic backport This PR is a backport produced by automation (does not trigger backporting) labels Jan 20, 2025

roberth added a commit that referenced this pull request Jan 21, 2025

Merge pull request #12287 from bryango/2.24-maintenance

f22359b

Backport git+file:./ fixes to 2.24 (#12107 + #12277)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fetchers/git: make relative path absolute for local repo #12107

fetchers/git: make relative path absolute for local repo #12107

bryango commented Dec 26, 2024 •

edited

Loading

aij left a comment

roberth left a comment

edolstra commented Dec 31, 2024

bryango commented Jan 1, 2025

bryango Jan 1, 2025

bryango commented Jan 4, 2025 •

edited

Loading

roberth commented Jan 4, 2025

bryango commented Jan 9, 2025

bryango commented Jan 10, 2025

roberth commented Jan 10, 2025

roberth commented Jan 15, 2025 •

edited

Loading

mergify bot commented Jan 15, 2025 •

edited

Loading

edolstra Jan 16, 2025

edolstra commented Jan 16, 2025

roberth commented Jan 16, 2025

bryango commented Jan 17, 2025 •

edited

Loading

fetchers/git: make relative path absolute for local repo #12107

fetchers/git: make relative path absolute for local repo #12107

Conversation

bryango commented Dec 26, 2024 • edited Loading

Motivation

Context

aij left a comment

Choose a reason for hiding this comment

roberth left a comment

Choose a reason for hiding this comment

edolstra commented Dec 31, 2024

bryango commented Jan 1, 2025

bryango Jan 1, 2025

Choose a reason for hiding this comment

bryango commented Jan 4, 2025 • edited Loading

roberth commented Jan 4, 2025

bryango commented Jan 9, 2025

bryango commented Jan 10, 2025

roberth commented Jan 10, 2025

roberth commented Jan 15, 2025 • edited Loading

mergify bot commented Jan 15, 2025 • edited Loading

✅ The pull request has been merged automatically

edolstra Jan 16, 2025

Choose a reason for hiding this comment

edolstra commented Jan 16, 2025

roberth commented Jan 16, 2025

bryango commented Jan 17, 2025 • edited Loading

bryango commented Dec 26, 2024 •

edited

Loading

bryango commented Jan 4, 2025 •

edited

Loading

roberth commented Jan 15, 2025 •

edited

Loading

mergify bot commented Jan 15, 2025 •

edited

Loading

bryango commented Jan 17, 2025 •

edited

Loading