Skip to content

Conversation

hendrikgit
Copy link
Contributor

This PR is to add proc setLastModificationTime*(file: string, t: times.Time) to os.

The proc setLastModificationTime is missing it's pragma, I was not sure what was required there.

toWinTime was added to avoid unixTimeToWinTime which expects CTime and CTime is not exported.

I have not figured out yet how to only set the last modification time with utimes, so I set the last access time too. Is that acceptable behaviour? The utimes documentation says only the whole times array can be null (in that case both times will be set to current time) or they each need to have a value.

@data-man
Copy link
Contributor

data-man commented Apr 8, 2018

Please, add a tests.

@GULPF
Copy link
Member

GULPF commented Apr 8, 2018

Looks good, but is there a reason why toFILETIME can't do the work of toWinTime as well? I know that unixTimeToWinTime is broken, but is there a common use case for getting a windows timestamp represented as a int64 instead of a FILETIME?

If not, I think it's better to just deprecate unixTimeToWinTime and refer to winlean.toFILETIME as a replacement. If the user really wants the result as a int64 then toFILETIME(unixTime, nanoseconds).rdFileTime() can be used.

@dom96
Copy link
Contributor

dom96 commented Apr 8, 2018

Looks good, but is there a reason why toFILETIME can't do the work of toWinTime as well? I know that unixTimeToWinTime is broken, but is there a common use case for getting a windows timestamp represented as a int64 instead of a FILETIME?

What would toFILETIME take as an argument though? It can't take Time, should Time just be passed as an int64 to it?

@GULPF
Copy link
Member

GULPF commented Apr 8, 2018

It could take unix seconds and nanoseconds as separate arguments:
proc toFILETIME(seconds: int64, nanoseconds: int): FILETIME
But this PR is probably more convenient in the end. Having toWinTime/fromWinTime does make some sense as a parallel to toUnix/fromUnix for the winapi.

@hendrikgit The test failures on AppVeyor/Travis is because of the JS target. The constants epochDiff and rateDiff used in toWinTime are defined inside a block which isn't executed for JS, so they need to be moved out of it.

@dom96
Copy link
Contributor

dom96 commented Apr 8, 2018

@GULPF If you're planning to have a toUnix and fromUnix in the times module as well then that's fine.

@GULPF
Copy link
Member

GULPF commented Apr 8, 2018

@dom96 toUnix/fromUnix already exists: https://nim-lang.org/docs/times.html#fromUnix,int64

@dom96
Copy link
Contributor

dom96 commented Apr 8, 2018

Oops, I missed it. Good to know.

@data-man
Copy link
Contributor

data-man commented Apr 8, 2018

Maybe to introduce OSFileTime type?
And toOSFileTime/fromOSFileTime.

The constants were moved out of the when defined(JS) block so that they
are alsways available in proc toWinTime.
proc toWinTime was moved above the # Deprecated procs comment. Best new
location seemed to be with the toUnix proc.
@Araq
Copy link
Member

Araq commented Apr 13, 2018

I think this is too special of a need for os.nim. -1 from me, sorry.

@data-man
Copy link
Contributor

@Araq
I also planned to add such proc.
And add saveTime parameter to httpclient.downloadFile.

@dom96
Copy link
Contributor

dom96 commented Apr 14, 2018

I think this is too special of a need for os.nim. -1 from me, sorry.

Is this in general to this PR of @data-man's comment?

Copy link
Contributor

@dom96 dom96 left a comment

Choose a reason for hiding this comment

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

One small comment, but otherwise this looks good.

if h == INVALID_HANDLE_VALUE: raiseOSError(osLastError())
var ft = t.toWinTime.toFILETIME
let res = setFileTime(h, nil, nil, ft.addr)
discard h.closeHandle
Copy link
Contributor

Choose a reason for hiding this comment

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

I would check this return value instead of discarding it. But it seems that most people disagree with me on this (I can see os doing a lot of this :)

@Araq Araq merged commit b1b5171 into nim-lang:devel Apr 16, 2018
@Kazark
Copy link

Kazark commented Aug 21, 2018

What's the plan for a release date for the next version of Nim? I'm excited to see this added as I have need for it. Also is the best way to get a prerelease build to build it myself?

@dom96
Copy link
Contributor

dom96 commented Aug 21, 2018

This week (hopefully)

@Kazark
Copy link

Kazark commented Aug 23, 2018

@dom96 super!

@hendrikgit hendrikgit deleted the setLastModificationTime branch September 2, 2018 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants