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

Add support for UTIME_NOW and UTIME_OMIT to TimeSpec #1103

Closed
wants to merge 2 commits into from
Closed

Add support for UTIME_NOW and UTIME_OMIT to TimeSpec #1103

wants to merge 2 commits into from

Conversation

lpetre
Copy link

@lpetre lpetre commented Aug 9, 2019

From the documentation:

If the tv_nsec field of one of the timespec structures has the special value UTIME_NOW, then the corresponding file timestamp is set to the current time. If the tv_nsec field of one of the timespec structures has the special value UTIME_OMIT, then the corresponding file timestamp is left unchanged. In both of these cases, the value of the corresponding tv_sec field is ignored.

Given the current API, there is no way to set tv_nsec to UTIME_NOW, since TimeSpec::nanoseconds mangles the value.

@lpetre
Copy link
Author

lpetre commented Aug 9, 2019

I don't think this works on OSX

src/sys/time.rs Outdated
@@ -144,6 +144,20 @@ impl TimeValLike for TimeSpec {
}

impl TimeSpec {
/// Makes a new `TimeSpec` with the special UTIME_NOW constant
#[inline]
Copy link
Member

Choose a reason for hiding this comment

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

No need for #[inline]. The compiler can figure that out for itself.

src/sys/time.rs Outdated
@@ -144,6 +144,20 @@ impl TimeValLike for TimeSpec {
}

impl TimeSpec {
/// Makes a new `TimeSpec` with the special UTIME_NOW constant
#[inline]
#[cfg(target_os = "linux")]
Copy link
Member

Choose a reason for hiding this comment

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

Plenty of OSes besides Linux have UTIME_NOW and UTIME_OMIT. Can you please enable all of them here? You may need to make a PR to libc first.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I'm happy to do that. I just don't know how to get that exhaustive list of OSes. Any suggestions?

Copy link
Member

Choose a reason for hiding this comment

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

There's no easy way. All of the open source OSes have online manpages; that's what I use. The hard one is OSX. I haven't found a good, authoritative source for OSX man pages.

Copy link
Member

Choose a reason for hiding this comment

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

I think you're still leaving out some OS support. FreeBSD, for one, defines these symbols. I haven't checked others. Can you please check their online man pages, and open a PR to libc to add support where it's missing?

Copy link
Author

Choose a reason for hiding this comment

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

I've got a PR open here: rust-lang/libc#1474

lpetre added 2 commits August 20, 2019 17:11
From the documentation:
```
If the tv_nsec field of one of the timespec structures has the special value UTIME_NOW, then the corresponding file timestamp is set to the current time. If the tv_nsec field of one of the timespec structures has the special value UTIME_OMIT, then the corresponding file timestamp is left unchanged. In both of these cases, the value of the corresponding tv_sec field is ignored.
```

Given the current API, there is no way to set tv_nsec to UTIME_NOW, since TimeSpec::nanoseconds mangles the value.
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

Don't forget to add a CHANGELOG entry.

target_env = "uclibc",
target_env = "wasi",
target_os = "android",
target_os = "emscripten",
Copy link
Member

Choose a reason for hiding this comment

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

You can add Dragonfly. Besides that, what targets are missing from this list? Does it not cover everything?

Copy link
Author

Choose a reason for hiding this comment

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

Opened another PR: rust-lang/libc#1487

@SteveLauC
Copy link
Member

Hi @lpetre, I just noticed that I created a duplicate PR of this one, since you come first, I would like to merge this and close mine, would you like to rebase your branch and finish this?

@SteveLauC
Copy link
Member

Alright, I just realized that you have deleted your Nix fork, then I will close this PR and do it in 1897, thanks for your interest in contributing to Nix!

@SteveLauC SteveLauC closed this Dec 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants