Guard uses of lutimes, for portability #7048

puffnfresh · 2022-09-15T05:57:56Z

puffnfresh · 2022-09-15T05:59:01Z

src/libfetchers/git.cc

  struct timeval times[2];
  times[0].tv_sec = touch_time;
  times[0].tv_usec = 0;
  times[1].tv_sec = touch_time;
  times[1].tv_usec = 0;

  return lutimes(path.c_str(), times) == 0;
+#else
+  return false;


Nix emits a warning when this returns false but I have assumed that's acceptable.

thufschmitt

Mh, that's sad (and apologies, at least some of these are my doing).

Dropping these is annoying because it can make things non-reproducible (and in a way that Nix might very much not like). Not sure whether there's a good way around this

thufschmitt · 2022-09-15T12:57:46Z

src/libutil/filesystem.cc

@@ -76,6 +77,7 @@ void createSymlink(const Path & target, const Path & link,
        if (lutimes(link.c_str(), times))
            throw SysError("setting time of symlink '%s'", link);
    }
+#endif


That guard (and the one below which acts on essentially duplicated code :/ ) is a bit annoying because it changes the semantics and affects reproducibility. Do you know whether there's a way to emulate lutimes if it's absent?

Or maybe we can just ignore setting the time for the symlinks if lutimes isn't available and pray that it doesn't matter in practice

futimes() is available since glibc 2.3. lutimes() is available since glibc 2.6, and is implemented using the utimensat(2) system call, which is supported since kernel 2.6.22.

@jugendhacker Does termux has the mentioned function ?

Ericson2314 · 2023-03-03T13:15:32Z

Triaged in the Nix team meeting:

@thufschmitt: lutimes is a syscall that's not available everywhere
- its for getting file timestamps while observing symlinks
- this is relevant for termux
@edolstra: don't really believe in autoconf checks and its philosophy
- should just have conditionals
no decision
assigned to @Ericson2314 to try a different approach and make the autoconf checks mandatory for now
- related: lutimes not optional #7045
- @Ericson2314: if we added a cross-compilation target for the platform to the CI, it would justify the additional CPP

nixos-discourse · 2023-03-03T13:35:19Z

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

https://discourse.nixos.org/t/2023-03-03-nix-team-meeting-minutes-37/25998/1

fricklerhandwerk · 2023-06-12T12:17:03Z

@Ericson2314 and me checked the code, and found #8495 (which you are welcome to implement, it's an almost trivial change).

Other than that, we agree with @thufschmitt. For any remaining occurrences of lutime we should do the following:

Factor out a utility function such that all uses of lutime go through it
Use lutime if possible, fall back to utime if the file in question is not a symlink, fail with an error message otherwise

Making this PR a draft until these changes are implemented.

Ericson2314 · 2023-06-14T18:11:09Z

#8504 removed the dead code, now there are just two lutimes to deduplicate left.

nixos-discourse · 2023-06-19T08:59:04Z

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

https://discourse.nixos.org/t/2023-06-12-nix-team-meeting-minutes-62/29315/1

fricklerhandwerk · 2024-01-21T20:03:10Z

This should be good to go, @thufschmitt @Ericson2314 do you want to take another look?

thufschmitt · 2024-02-01T08:30:50Z

src/libutil/file-system.hh

+ * Whether or not the path is a symlink is taken from the mode of `st`
+ * in fallback code.


I don't think that precision really makes sense without the context of that PR (what's “fallback mode”?)

Besides that, I don't think we should trust st to tell us whether the file is a symlink or not. Barring possible race conditions (which I think we can't avoid any way), there's no guaranty that st refers to the path at path (might have been a totally made up struct just to set the time, or taken from another file). We should rather re-check within the function (std::filesystem::is_symlink being our friend here I guess)

fallback mode is without lutimes, with just utimes (yes, this should be reworded)

Besides that, I don't think we should trust st to tell us whether the file is a symlink or not.

Note that this is not a new thing. I agree in principle, but how would you update canonicaliseTimestampAndPermissions in practice? It is, already, very geared about working from a prior stat (in a context that we don't think things would change without them looking.

Unless we want to scope creep and overhaul canonicaliseTimestampAndPermissions, I think it would be good to just document the race condition on this function and say callers must be confident st is up to date.

Note that this is not a new thing. I agree in principle, but how would you update canonicaliseTimestampAndPermissions in practice? It is, already, very geared about working from a prior stat (in a context that we don't think things would change without them looking.

That's indeed already the case for canonicaliseTimestampAndPermissions, but I'd rather not also make it the case here when we can just replace the S_ISLNK(st.st_mode) calls by std::filesystem::is_symlink(path)

OK this is done now. the struct stat way still exists, but is not forced upon everyone else.

Ericson2314

@thufschmitt's request is addressed. All the compat is a bit gnarly, but future improvements are possible.

addressed

puffnfresh commented Sep 15, 2022

View reviewed changes

thufschmitt suggested changes Sep 15, 2022

View reviewed changes

fricklerhandwerk added the bug label Mar 3, 2023

thufschmitt assigned Ericson2314 Mar 3, 2023

fricklerhandwerk mentioned this pull request Jun 12, 2023

Remove dead code for createSymlink and replaceSymlink #8495

Closed

fricklerhandwerk marked this pull request as draft June 12, 2023 12:17

fricklerhandwerk added the build-problem Nix fails to compile or test; also improvements to build process label Jun 12, 2023

salpelter mentioned this pull request Jun 12, 2023

Remove dead code #8504

Merged

8 tasks

Ericson2314 force-pushed the lutimes branch from e467753 to f0ee1f4 Compare November 30, 2023 06:27

github-actions bot added the fetching Networking with the outside (non-Nix) world, input locking label Nov 30, 2023

Ericson2314 force-pushed the lutimes branch from f0ee1f4 to 2d223de Compare November 30, 2023 06:38

fricklerhandwerk marked this pull request as ready for review January 21, 2024 20:02

fricklerhandwerk requested a review from edolstra as a code owner January 21, 2024 20:02

fricklerhandwerk requested a review from thufschmitt January 21, 2024 20:02

thufschmitt previously requested changes Feb 1, 2024

View reviewed changes

Fix error in the no-GC build

05580a3

Ericson2314 force-pushed the lutimes branch 2 times, most recently from fa1ca2e to 9642d2d Compare June 24, 2024 21:28

Guard uses of lutimes, for portability

5be44d2

Ericson2314 force-pushed the lutimes branch from 9642d2d to 5be44d2 Compare June 24, 2024 21:35

Ericson2314 enabled auto-merge June 24, 2024 21:37

Ericson2314 approved these changes Jun 24, 2024

View reviewed changes

Ericson2314 merged commit d02d38f into NixOS:master Jun 25, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Guard uses of lutimes, for portability #7048

Guard uses of lutimes, for portability #7048

puffnfresh commented Sep 15, 2022

puffnfresh Sep 15, 2022 •

edited

Loading

thufschmitt left a comment

thufschmitt Sep 15, 2022

Et7f3 Feb 3, 2023

Et7f3 Feb 3, 2023

Ericson2314 commented Mar 3, 2023 •

edited by fricklerhandwerk

Loading

nixos-discourse commented Mar 3, 2023

fricklerhandwerk commented Jun 12, 2023

Ericson2314 commented Jun 14, 2023

nixos-discourse commented Jun 19, 2023

fricklerhandwerk commented Jan 21, 2024

thufschmitt Feb 1, 2024

Ericson2314 Feb 1, 2024

thufschmitt Feb 1, 2024

Ericson2314 Jun 24, 2024 •

edited

Loading

Ericson2314 left a comment

		* Whether or not the path is a symlink is taken from the mode of `st`
		* in fallback code.

Guard uses of lutimes, for portability #7048

Guard uses of lutimes, for portability #7048

Conversation

puffnfresh commented Sep 15, 2022

puffnfresh Sep 15, 2022 • edited Loading

Choose a reason for hiding this comment

thufschmitt left a comment

Choose a reason for hiding this comment

thufschmitt Sep 15, 2022

Choose a reason for hiding this comment

Et7f3 Feb 3, 2023

Choose a reason for hiding this comment

Et7f3 Feb 3, 2023

Choose a reason for hiding this comment

Ericson2314 commented Mar 3, 2023 • edited by fricklerhandwerk Loading

nixos-discourse commented Mar 3, 2023

fricklerhandwerk commented Jun 12, 2023

Ericson2314 commented Jun 14, 2023

nixos-discourse commented Jun 19, 2023

fricklerhandwerk commented Jan 21, 2024

thufschmitt Feb 1, 2024

Choose a reason for hiding this comment

Ericson2314 Feb 1, 2024

Choose a reason for hiding this comment

thufschmitt Feb 1, 2024

Choose a reason for hiding this comment

Ericson2314 Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Ericson2314 left a comment

Choose a reason for hiding this comment

puffnfresh Sep 15, 2022 •

edited

Loading

Ericson2314 commented Mar 3, 2023 •

edited by fricklerhandwerk

Loading

Ericson2314 Jun 24, 2024 •

edited

Loading