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.

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

Implement setModificationTime counterpart to getModificationTime #13

Closed
hvr opened this issue Jan 15, 2015 · 10 comments
Closed

Implement setModificationTime counterpart to getModificationTime #13

hvr opened this issue Jan 15, 2015 · 10 comments
Assignees
Labels
type: a-feature-request This is a request for a new feature.
Milestone

Comments

@hvr
Copy link
Member

hvr commented Jan 15, 2015

For POSIX it should be a no-brainer, and for Win32 it seems possible as well:

@hvr hvr added type: a-feature-request This is a request for a new feature. help wanted labels Jan 15, 2015
@redneb
Copy link
Contributor

redneb commented Feb 16, 2015

I have already written some code that does that. In fact I almost submitted a patch a year ago but what stopped me is the fact that there is an issue with setting modification dates: you cannot always set it independently of the last access time. In particular:

  • Windows supports setting the file times independently.
  • Linux also supports that via the utimensat(2) call, but the unix package does not expose that functionality.
  • Other unix systems do not (currently) support utimensat so they cannot set file times independently.

So if we want our new function to have the following signature:

setModificationTime :: FilePath -> UTCTime -> IO ()

we have the following options regarding the atime:

  1. Update the atime by setting it equal to the mtime.
  2. Update the atime by setting to the current time (requires making an extra system call).
  3. Preserve the atime. On systems that do not support setting file times independently, this requires an extra system call to retrieve the current atime. In that case, what do we do about systems that have utimensat, do we call utimensat via FFI to avoid the extra call?

There is also a slightly different option (call it option 4): introduce the following two functions:

getAccessTime :: FilePath -> IO UTCTime 
setFileTimes :: FilePath -> UTCTime -> UTCTime -> IO ()

My vote is for 3 (with FFI for systems with utimensat) or 4.

@Rufflewind
Copy link
Member

In that case, what do we do about systems that have utimensat, do we call utimensat via FFI to avoid the extra call?

It should be considered a feature request in unix. If they accept it then we can make use of the new feature and resort to the two-step fallback solution only if unix is doesn't support it, i.e. getFileStatus followed by setFileTimesHiRes. This allows directory to avoid the FFI import.

The downside is that users who have older versions of unix will not get the benefit of atomicity/speed on platforms that support utimensat. However, I don't think that's a major issue – if it were they we can't use any fallback at all!

Hence, I favor option 3 but it's not exclusive with option 4 – it could be added in the future if there's a need for it.

@redneb
Copy link
Contributor

redneb commented Feb 25, 2015

When I implemented setFileTimesHiRes for unix, I contemplated giving it the following signature:

setFileTimesHiRes :: FilePath -> Maybe POSIXTime -> Maybe POSIXTime -> IO ()

which would have required doing multiple syscalls on platforms without utimensat, but in the end I decided to drop the Maybes because I felt that unix is a low-level package and that each function in it should correspond to a single syscall if possible.

It should be considered a feature request in unix. If they accept it then we can make use of the new feature and resort to the two-step fallback solution only if unix is doesn't support it, i.e. getFileStatus followed by setFileTimesHiRes. This allows directory to avoid the FFI import.

I don't see how this could be done in unix in a reasonable way:

  • Would it export a function like the one above but only if utimensat is supported? That's a bad idea and there's no other such case in unix.
  • Would it always export such a function and throw an error if it's not supported? Then we would have to catch that exception and do the two-step fallback in directory at runtime, that's not very elegant.
  • Have unix do the two-step fallback with #ifdefs? unix is a low level package.
  • Have unix export something else, like something involving a Maybe? That's also ugly.
  • Something else?

Remeber, unix alread exports: setFileTimes, setFileTimesHiRes, setFdTimesHiRes, and setSymbolicLinkTimesHiRes. I don't think adding one more function in this family or modifying one of them is a good idea.

@Rufflewind
Copy link
Member

Here's what I'm trying to avoid: to check whether utimensat is available in directory. In principle, unix knows whether utimensat is available for the current system, but it doesn't expose that to the user.

Would it export a function like the one above but only if utimensat is supported?

That's a bad idea because there's no easy way for the user to find out programmatically.

Would it always export such a function and throw an error if it's not supported? Then we would have to catch that exception and do the two-step fallback in directory at runtime, that's not very elegant.

It's not the most elegant what other options are there? (Though I think it makes more sense to throw an IOError rather than a plain error.)

Have unix do the two-step fallback with #ifdefs?

That was my original plan, but I realize why that may not be a good idea.

Have unix export something else, like something involving a Maybe?

It feels like the morally right approach, but it's also ugly and awkward to use (worse if fromJust is removed):

Maybe (FilePath -> Maybe POSIXTime -> Maybe POSIXTime -> IO ())

A pure boolean flag that indicates whether utimensat is supported would be a nice alternative to, but it looks a bit out of place.

@hvr
Copy link
Member Author

hvr commented Feb 25, 2015

fyi, regarding non-portable API calls: haskell/unix#23

@Rufflewind
Copy link
Member

Ah, so directory can make use of the macros from unix via HsConfig.h. In that case, the problem isn't quite as awkward as I thought it'd be. I would still rather have unix define the FFI for utimensat though.

So, is there a consensus on using unsupportedOperation now?

@hvr
Copy link
Member Author

hvr commented Mar 1, 2015

So, is there a consensus on using unsupportedOperation now?

Lemme put it this way, I asked the CLC to discuss it a few days ago (but no final decision yet though as of writing): https://groups.google.com/d/topic/haskell-core-libraries/hmeSVXBcM1I/discussion

@Rufflewind
Copy link
Member

The more I think about it, the more I think Maybe is the cleanest solution. I think with INLINABLE GHC should be able to optimize out the check (not like it makes any difference anyway, since system calls are already rather slow). Compare to all the other solutions, the only downside is a slight loss in usability.

Using #define has the downside of requiring CPP and introducing conditionally-compiled code, which makes it easy to write code that doesn't compile on another platform.


Regardless however, I think unsupportedOperation should be preferred over error since it's more descriptive.

@hvr
Copy link
Member Author

hvr commented Mar 1, 2015

Assuming there's a statically known Maybe (i.e. it's a pure CAF value), it doesn't allow your code to fail to compile statically if an operation is missing (which is what the #ifdef variant allows for). Moreover, there's no smooth upgrade path between always available and conditionally available operation, as you are forced to change the type-signature to a Maybe-wrapped type-signature, thereby breaking a lot of existing code. So tbh, I see no way we could upgrade all existing unix operation to Maybe-wrapped versions w/o duplicating the API (each API duplication adds up exponentially sadly)

(Fwiw, I've been missing the equivalent of C++'s static_assert... so far I had to resort to TH to emulate it, but it's quite overkill to have to use TH for that IMO)

@Rufflewind Rufflewind added the type: b-discussion This is a discussion with unclear objectives. label Mar 3, 2015
@Rufflewind Rufflewind added this to the 1.2.3.0 milestone Mar 11, 2015
@Rufflewind
Copy link
Member

Here's a WIP (Windows version is untested). [Edit: branch no longer exists]

Aside from having a more general setFileTimes it would've been nice to have access to the C stuff from the unix library (CTimeSpec, toCTimeSpec, AT_FDCWD). But anyway, I just went ahead and reimplemented them in a new internal module with hsc.

@Rufflewind Rufflewind self-assigned this May 26, 2015
Rufflewind added a commit to Rufflewind/directory that referenced this issue May 30, 2015
The unix package does not have a general setFileTimesHiRes that allowed
setting mtime separately from atime.  Therefore, we import the foreign
utimensat function in the new Internal module (hsc), which also involves
implementing struct timespec.

Fixes haskell#13.
@Rufflewind Rufflewind removed type: b-discussion This is a discussion with unclear objectives. help wanted labels May 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: a-feature-request This is a request for a new feature.
Projects
None yet
Development

No branches or pull requests

3 participants