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.

Sign up for GitHub

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

Jump to bottom

Restore builtins.pathExists behavior on broken symlinks #9985

Merged
merged 2 commits into from
Feb 16, 2024
Merged

Restore builtins.pathExists behavior on broken symlinks #9985

merged 2 commits into from
Feb 16, 2024

Conversation

alois31
Copy link
Contributor

@alois31 alois31 commented Feb 10, 2024 •
edited
Loading

Commit 83c067c changed builtins.pathExists to resolve symlinks before checking for existence. Consequently, if the path refers to a symlink itself, existence of the target of the symlink (instead of the symlink itself) was checked. Restore the previous behavior by skipping symlink resolution in the last component.

Motivation

Restore backwards compatibility.

Context

Fixes: #9901

Priorities and Process

Add 👍 to pull requests you find important.

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

@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Feb 10, 2024
@alois31

This comment was marked as outdated.

Sign in to view
@Ericson2314
Copy link
Member

Per #9585 (comment) actually 2.18 is wrong, and the newer behavior that makes more sense is also the original older behavior.

@Ericson2314 Ericson2314 changed the title Restore builtins.pathExists behavior on broken symlinks Restore builtins.pathExists behavior on broken symlinks Feb 12, 2024
@alois31 alois31 marked this pull request as draft February 12, 2024 18:52
@alois31
Restore builtins.pathExists behavior on broken symlinks
89e21ab 
Commit 83c067c changed `builtins.pathExists`
to resolve symlinks before checking for existence. Consequently, if the path
refers to a symlink itself, existence of the target of the symlink (instead of
the symlink itself) was checked. Restore the previous behavior by skipping
symlink resolution in the last component.
@alois31
Copy link
Contributor Author

alois31 commented Feb 13, 2024 •
edited
Loading

The obviously correct behavior for the directory thing is now implemented again, along with a test to prevent re-regression.

@alois31 alois31 marked this pull request as ready for review February 13, 2024 17:14
@Ericson2314
Copy link
Member

@alois31 hmm so I am wondering whether the trailing slash should make a difference, as I believe is how its implemented.

One way to rationalize the trailing . behavior is, until the . is removed, the symlink is an ancestor component.

@alois31
Copy link
Contributor Author

alois31 commented Feb 13, 2024

Yeah, that would be my reasoning as well. The comment "SourcePath doesn't know about trailing slash." applies here too.

@Ericson2314
Copy link
Member

Ah I see. I guess I am contemplating that coerceToPath should instead optionally resolve symlinks.

@alois31
Copy link
Contributor Author

alois31 commented Feb 13, 2024

Interesting idea. Investigating this exposed another regression: coerceToPath in addition to stripping the slash also resolves .. improperly (without taking into account symlinks).

@Ericson2314
Copy link
Member

@alois31 my understanding is that in fact for many years .. was resolved incorrectly, but yeah we already started fixing that bug (some nastiness in my personal NixOS setup was relying on it).

@Ericson2314
Copy link
Member

Ericson2314 commented Feb 13, 2024 •
edited
Loading

Check out #9881. You can use the callback and a SourceAccessor to resolve symlinks properly.

@Ericson2314 Ericson2314 mentioned this pull request Feb 13, 2024
Merged
@thufschmitt
Copy link
Member

Discussed during the Nix maintainers meeting on 2024-02-12.
@Ericson2314 will help move this forward (well, he already started).

#9985 (comment)

#9585 (comment)

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-02-12-nix-team-meeting-minutes-123/39775/1

@alois31
Copy link
Contributor Author

alois31 commented Feb 15, 2024

Check out #9881. You can use the callback and a SourceAccessor to resolve symlinks properly.

Hm, CanonPath plus SourceAccessor begins to smell an awful lot like SourcePath. I now wonder if CanonPath should just be merged into SourcePath…

@Ericson2314
Copy link
Member

@alois31 CanonPath should say separate, but yes this way of making a CanonPath could also just be a way of making a SourcePath (instead of just temporary using the accessor, it would hold onto it.)

@alois31
Copy link
Contributor Author

alois31 commented Feb 15, 2024

@Ericson2314 Why should it stay separate, because it represents some kind of "abstract" path (like the "path" type in the Nix language)? Then at least the things not belonging into that (for example the second copy of the symlink resolution code) should be merged into SourcePath.

@Ericson2314
Copy link
Member

Yes, and yes.

Ericson2314
Ericson2314 approved these changes Feb 16, 2024
View reviewed changes
Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

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

The rest of the Nix team pointed out we want a bug fix that we can backport, so I am approving this as-is after all. Hopefully @alois31 you are still interested working on the other solution in a follow-up :).

@Ericson2314 Ericson2314 added the backport 2.20-maintenance Automatically creates a PR against the branch label Feb 16, 2024
@Ericson2314 Ericson2314 mentioned this pull request Feb 16, 2024
Closed
@Ericson2314 Ericson2314 added the backport 2.19-maintenance Automatically creates a PR against the branch label Feb 16, 2024
@Ericson2314 Ericson2314 merged commit d53c890 into NixOS:master Feb 16, 2024
10 checks passed
@github-actions GitHub Actions
Copy link

Backport failed for 2.19-maintenance, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.19-maintenance
git worktree add -d .worktree/backport-9985-to-2.19-maintenance origin/2.19-maintenance
cd .worktree/backport-9985-to-2.19-maintenance
git switch --create backport-9985-to-2.19-maintenance
git cherry-pick -x 89e21ab4bd1561c6eab2eeb63088f4e34fa4059f e27b7e04bf38c1fdf342d6e15b2c003ca9b92cb1

@github-actions GitHub Actions
Copy link

Successfully created backport PR for 2.20-maintenance:

@github-actions github-actions bot mentioned this pull request Feb 16, 2024
Merged
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-02-16-nix-team-meeting-minutes-124/39870/1

@infinisil
Copy link
Member

@Ericson2314 (regression started in 2.20, so 2.19 backport not necessary)

@Ericson2314
Copy link
Member

Ah yes, I misread #9901.

@Ericson2314 Ericson2314 removed the backport 2.19-maintenance Automatically creates a PR against the branch label Feb 16, 2024
edolstra
edolstra reviewed Feb 16, 2024
View reviewed changes
src/libexpr/primops.cc
@@ -115,7 +115,7 @@ StringMap EvalState::realiseContext(const NixStringContext & context)
return res;
}

static SourcePath realisePath(EvalState & state, const PosIdx pos, Value & v, bool resolveSymlinks = true)
static SourcePath realisePath(EvalState & state, const PosIdx pos, Value & v, std::optional<SymlinkResolution> resolveSymlinks = SymlinkResolution::Full)
Copy link
Member

Choose a reason for hiding this comment

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

BTW, instead of an std::optional, wouldn't it be cleaner to have a SymlinkResolution::None alternative?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's see how much of that footgun remains at all after working on the nicer fix.

srid added a commit to srid/nixos-config that referenced this pull request Mar 26, 2024
@srid
Don't use unstable nix CLI
380b12e 
Because it broke colmena,
zhaofengli/colmena#190

Ref:
- NixOS/nix#9497
- NixOS/nix#9901
- NixOS/nix#9985
@Ericson2314
Copy link
Member

@alois31 are you still interested in working on the follow-up cleanup?

@alois31
Copy link
Contributor Author

alois31 commented Apr 3, 2024

I fear I will not be able to work on this, sorry. The task seems to be much bigger than initially anticipated and I'm not up to it.

tebowy pushed a commit to tebowy/nix that referenced this pull request Jul 11, 2024
Merge pull request NixOS#9985 from alois31/symlink-resolution
b14f88e 
Restore `builtins.pathExists` behavior on broken symlinks

(cherry picked from commit d53c890)

===

note that this variant differs markedly from the source commit because
we haven't endured quite as much lazy trees.

Change-Id: I0facf282f21fe0db4134be5c65a8368c1b3a06fc
huang12zheng pushed a commit to huang12zheng/nixos-config that referenced this pull request Dec 5, 2024
@srid
Don't use unstable nix CLI
de240f6 
Because it broke colmena,
zhaofengli/colmena#190

Ref:
- NixOS/nix#9497
- NixOS/nix#9901
- NixOS/nix#9985
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edolstra edolstra edolstra left review comments

@Ericson2314 Ericson2314 Ericson2314 approved these changes

@roberth roberth Awaiting requested review from roberth roberth is a code owner

Assignees

@Ericson2314 Ericson2314

Labels
backport 2.20-maintenance Automatically creates a PR against the branch bug with-tests Issues related to testing. PRs with tests have some priority
Projects
Nix team
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

builtins.pathExists returns false on symlinks that point to non-existent paths
6 participants
@alois31 @Ericson2314 @thufschmitt @nixos-discourse @infinisil @edolstra