-
-
Notifications
You must be signed in to change notification settings - Fork 31.3k
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 st_*time_ns fields to os.stat(), add ns keyword to os.*utime*(), os.*utimens*() expects a number of nanoseconds #58335
Comments
Following on from Guido's rejection of PEP-410: This bug is the proposed hammering-out space for how to preserve exact metadata for st_atime / st_mtime between os.stat and os.utime. (Yes, there's already bpo-11457 -- but that went pretty far down the rabbit hole of Decimal. I thought maybe a fresh start would be best.) |
Guido proposed st_atime_ns et al. I'll make an alternate proposal. I'd like to avoid tying ourselves to ns resolution. As MvL said: "I don't want to deal with this issue *again* in my lifetime". If all we want is to facilitate copying the exact metadata from os.stat to os.utime, then we don't really care about conveniently modifying the timestamp. So I propose we use MvL's suggestion of a tuple of ints. Either (numerator, denominator) or (seconds, fractional_numerator, fractional_denominator). (These are mentioned in PEP-410 under the heading "Tuple of ints" as options A and B respectively.) Name the fields with the suffix "_exact" (e.g. st_mtime_exact). os.stat and its ilk produce it; os.utime and its ilk consume it. If people want to monkey with the values and do math, let 'em--consenting adults. |
OTOH, it seems that Guido is very much in favor of hard-coding ns resolution in the API, claiming that expectations of even finer resolution are academic. While I still disagree (the *very* same argument was brought up for ms resolution ten years ago), I think that practicality beats purity here: people apparently can deal with an fixed scale much better than with a floating one. So it may be better to meet the apparent intuition of the majority than follow the purity route. |
Is it totally insane to think about using a float subclass with an additional attribute instead of a tuple? |
@rdm: yes, that's insane for this purpose. The size of the solution doesn't match the size of the problem (the problem is non-existent for most people). @larry: That seems unnecessarily general. I take full responsibility for fixing the precision at ns. And note that I *am* giving you a way out -- I'm establishing a clear pattern, and if someone really, desperately wants ps or fs, it's completely obvious how to change the API again. But I bet you a case of beer that won't happen in our lifetimes (or in Benjamin's). So the dynamic precision is just unnneeded complexity. |
I suggest that publishing nanoseconds as a plain int would be a nasty API. Consider what it would do to os.utime: if isinstance(mtime, int):
# must be st_mtime_ns, it's in nanoseconds, use as-is
value = mtime
else:
assert isinstance(mtime, float)
# must be st_mtime, it's in seconds, multiply by a billion
value = mtime * 1000000000 Have we ever published an API that treated a parameter as two wildly different numbers based solely on whether the parameter was an int or a float? |
(D'oh. My pseudo-code should have said value = int(mtime * 1000000000) for that second assignment. Hopefully my point was still clear.) |
How about a separate float attribute for the fractional part (in seconds)? This gives a femtosecond precision (assuming a 50 bits mantissa). The utime() could accept a (integral part, fractional part) tuple to set a timestamp without losing bits. (PHP does something similar, but badly, with its microtime() function: |
@pitrou: What would that look like when passed in to os.utime? |
>>> st = os.stat('LICENSE')
>>> st.st_mtime
1330108216.7984242
>>> st.st_mtime_frac
0.798424152
>>> tup = st.st_mtime, st.st_mtime_frac
>>> os.utime('LICENSE', (tup, tup)) Of course, the fact that utime takes a (atime, mtime) tuple makes this a bit cumbersome. |
No, it wouldn't. Please re-read Guido's proposal. If you want to
No, and Guido is on the record for objecting such APIs. Hence the |
Thanks Martin. I'd be happy with nsec instead of ns. |
My mistake! That ought to teach me to bound out of bed Sunday morning with my "brilliant realization". (But it probably won't.) I volunteer to implement this, with the new custom class for the os.stat object. I'll have it done no later than the PyCon sprints next month. For the record, I actually prefer "ns". "s" is the SI standard abbreviation for second (as is "n" for nano-): |
Let's go with ns= them. It's also what POSIX uses (although I initially had problems parsing "futimens", reading it as foo-timen-z, when it really is eff-youtime-en-es) |
Because the use case is to copy the access and modification time from a file to another, I would prefer to use the timespec structure: (sec: int, nsec: int). st_atime_ns and st_mtime_ns fields would be added to os.stat() structure: int in range [0; 999999999]. Copy atime and mtime from src to dst: st = os.stat(src)
atime = (math.floor(st.st_atime), st.st_atime_ns)
mtime = (math.floor(st.st_mtime), st.st_mtime_ns)
os.utime(dst, (atime, mtime)) os.utime() would accept int, float or (sec: int, nsec: int) for atime and mtime. Examples: os.utime(name, (1, 1))
os.utime(name, (1.0, 1.0))
os.utime(name, ((1, 0), (1, 0))) |
That's not future-proof for when we have better-than-nanosecond |
Actually, I'm hoping that by the time we get better than nanosecond resolution, we can also switch to 128-bit floats, and then the existing st_[acm]time field will become the preferred representation once more. What goes around comes around! |
What if your hope isn't fulfilled? That doesn't sound like a reasonable |
Well, Guido has already nixed future-proof formats, see his comments above: "I take full responsibility for fixing the precision at ns." So hope is all I have left. |
I don't think Guido is *against* future-proof formats per se, he's The proposal I made (a (integral part, float fractional part) tuple) |
I grant you that (int, float) is probably, theoretically better than (int, int). But it's academic as Guido has ruled against anything but a straight int representing nanoseconds, and I doubt he's gonna change his mind. |
Any solution involving tuple is too ugly. Please stick with the plan. |
Why not let Guido speak out? |
The following solution might be compatible with Guido's suggestion: os.stat(path).st_atime_ns -> nanoseconds_since_epoch_as_int atime = os.stat(path).st_atime_ns
mtime = os.stat(path).st_mtime_ns
os.utime(path, (atime, mtime), resolution="ns") |
I don't see how that's better than os.utime(path, ns=(atime, mtime)) If you think that in the future you could add resolution="fs", well, you could just as easily add fs=(atime, mtime). |
FWIW, +1 on using the *name* of the keyword arg to define the format/resolution of the argument. It's simple and clear right now, and is easily updated to handle higher resolutions in the future. |
I'm lost in all issues related to os.stat/utime and nanosecond, here is a list: |
Anybody else? I guess I'm gonna juuuust miss Alpha 3, but if nobody has any other objections I'll check this in today (Thursday). If you want me to hold off just let me know. |
It's because it's a git-style diff, so it includes no base revision, |
FYI, Martin was replying to Guido's comment from more than a month ago, when I posted revision #2 of my first patch series. By sheer coincidence I posted revision #2 of a new patch series yesterday. But Reitveld worked fine for that! Anyway--no comments? Normally I find the patch review process akin to getting pecked to death by ducks. It's hard to believe this one might go in after only one revision. Somebody pinch me! |
Failure on x86 OpenIndiana 3.x: FAIL: test_stat_attributes (test.test_os.StatAttributeTests) Traceback (most recent call last):
File "/export/home/buildbot/32bits/3.x.cea-indiana-x86/build/Lib/test/test_os.py", line 240, in test_stat_attributes
self.check_stat_attributes(self.fname)
File "/export/home/buildbot/32bits/3.x.cea-indiana-x86/build/Lib/test/test_os.py", line 199, in check_stat_attributes
self.assertEqual(floaty, nanosecondy)
AssertionError: 133572199178458 != 133572199178457 |
New changeset bba131e48852 by Larry Hastings in branch 'default': |
@Haypo: Thanks for pointing that out buildbot failure! The OpenIndiana buildbot was bit by a rounding error. I fixed it by adding in a fudge factor it--I snuck in one last change just now. I weakened the unit test as follows:
+ self.assertAlmostEqual(floaty, nanosecondy, delta=2) Also: I'm leaving this issue open for now, to remind myself to add ns= to utimensat and futimesat if they're still alive on June 15th. |
How do we have rounding issue with only integers? |
We don't! The test that failed compares the ns_[amc]time and ns_[amc]time_ns fields to ensure they're roughly equivalent. Note that the former fields are floats, which by now ought not to be news to you. |
ERROR: test_copy2_symlinks (test.test_shutil.TestShutil) Traceback (most recent call last):
File "/export/home/buildbot/64bits/3.x.cea-indiana-amd64/build/Lib/test/test_shutil.py",
line 328, in test_copy2_symlinks
shutil.copy2(src_link, dst, symlinks=True)
File "/export/home/buildbot/64bits/3.x.cea-indiana-amd64/build/Lib/shutil.py",
line 193, in copy2
copystat(src, dst, symlinks=symlinks)
File "/export/home/buildbot/64bits/3.x.cea-indiana-amd64/build/Lib/shutil.py",
line 157, in copystat
utime_func(dst, ns=(st.st_atime_ns, st.st_mtime_ns))
TypeError: _nop() got an unexpected keyword argument 'ns' |
New changeset cdc4e0f8135d by Larry Hastings in branch 'default': |
bba131e48852 causes crashes on Windows. The attached patch fixes the crash and makes test_os pass for me. However, using "PyErr_ExceptionMatches(PyExc_RuntimeError)" to check whether to try again using narrow strings is ugly. Maybe utime_read_time_arguments() should be changed to have three possible return values. |
I appreciate the feedback, and the patch. And I agree--we should be able to find a better fix than that particular band-aid. Can we hold off on checking in a patch for now? TBH I don't understand why it should crash, and therefore how your patch helps. Trying again using narrow strings should always work; indeed, the code did that before I touched it. Can you describe how it crashes? (p.s. Considering that I can't test on Windows myself, I'm pretty happy that the code works as well as it does!) |
The important part of the patch is the removal of the "!" in if (!utime_read_time_arguments(&ua)) { Without that change, if utime_read_time_arguments(&ua) fails then the unicode path is wrongly chosen. Then PyUnicode_AsUnicode(upath) is called when upath has not been initialized. |
Without the check for RuntimeError
raises TypeError("TypeError: 'str' does not support the buffer interface") because we have fallen through to the narrow path. The correct error is RuntimeError("utime: you may specify either 'times' or 'ns' but not both") |
Let me recap, just to make sure I have it straight. There are two errors on Windows: * The ! on (what is currently) line 3770 is wrong:
if (!utime_read_time_arguments(&ua)) {
For the former, obviously removing the ! is correct. But I like your idea of making the utime_read_time_argument() return value tell you what happened; that's what the Windows code needs to know. So here it is! Please see the attached patch. |
That's right. The patch looks good and passes for me on Windows. |
New changeset fc5d2f4291ac by Larry Hastings in branch 'default': |
There is another problem causing a fatal error in test_posix on Unix. The attached patch fixes it: *ua->path should be decrefed not ua->path. |
Looks good to me. You're a core contributor, yes? If not let me know and I'll commit it. Though I must admit I'm baffled how I haven't seen that crash. I've run the unit tests a zillion times on this patch. |
I will commit.
Were you running test_posix or only test_os? |
By "the unit tests" I meant I ran the whole suite: Lib/test/regrtest.py. I know that runs test_os, and I assume it runs test_posix too. I just ran test_posix by hand and it passed. I'm developing on Linux (64-bit) in case that helps. |
I tested it on 32 bit Linux. I have committed it, but I forgot to put the issue number in the commit message. |
Revision 4deb7c1f49b9 |
New changeset 709850f1ec67 by Larry Hastings in branch 'default': |
New changeset 05274ab06182 by Larry Hastings in branch 'default': |
Closing this, as I have now removed os.utimensat and os.futimesat. As well as os.futimens, os.futimes, and os.lutimes. And in fact retooled os.utime in a pretty major way. (See bpo-14626.) |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: