fix cargo not doing anything when the input and output mtimes are equal #5919
Conversation
That's a problem as the input may have changed in that same second but after the output got generated!
|
(rust_highfive has picked a reviewer for you, use r? to override) |
|
Note that I do not fully understand why this fixes the problem. The comparison is (from what I can see) at type Also, the Notice the "now |
|
That'd be a non-deterministic test case, which I am not sure how to even pull off. But yeah, would be nice indeed.^^ |
|
I extended logging to always print the timestamps it is comparing, and indeed I am seeing However, there has definitely been a |
|
Given this fix, couldn't you trigger |
|
More debugging reveals The Ah it says right in the code what that is, it's the |
|
Oy, this breaks on apple Essentially, the old code had a race condition that could lead to spuriously not rebuilding when we should. The new code has a race that could lead to spuriously rebuilding when we should not... we can only have one or the other, and the latter is clearly less buggy. |
|
I have a problem implementing the test... first I used this to "touch" the file: let _ = OpenOptions::new()
.write(true)
.open(root.join("src/main.rs"))
.unwrap();but then the test fails even with the fix, so I guess it doesn't really touch? {
let mut file = OpenOptions::new()
.append(true)
.open(root.join("src/main.rs"))
.unwrap();
file.write(" ".as_bytes()).unwrap();
}but now the test passes even without the fix... EDIT: let now = FileTime::from_system_time(SystemTime::now());
set_file_times(&path, now, now).unwrap();to no avail. The mtime changes, but the bug does not occur. Maybe it has to do with this being on EDIT2: Nope it tests inside the |
|
I managed to make a proper test by calling but that won't work on Windows... |
|
I looked at I give up on adding a test case. |
|
The comment here I think isn't quite right in the sense of, as you've found, the mtime comparison is comparing nanoseconds, not just seconds. I believe that some filesystems don't support nanosecond precision which means that the granularity may be seconds. I'm not 100% certain on the fix here because it seems like it may cause rebuilds in some situations perhaps? |
|
My filesystem however supports nanosecond precision. I am suspecting some kind of timestamp caching somewhere to avoid querying the system clock too much.
Yes. In case the timestamps are equal, we just cannot know which event happened first. We have two choices:
|
|
If you look at this assuming we just had 1-second granularity, then I think it is quite clear that we HAVE to rebuild when in doubt. Otherwise someone who compiles and then quickly edits in the same second will have their edits ignored, even if they call We are using nanosecond granularity, but that does not change the fact that rebuilding on equal timestamps is (IMO) the only correct strategy. Nanosecond timestamps should mean that we never see equal timestamps, but alas, we do. And, as you said, there are file systems that only support 1-second granularity, and then again I find "rebuild on equal timestamp" to be the only reasonable choice. |
|
I definitely agree that with less-than-nanosecond precision this patch makes sense, but what I'm trying to make sense of is that it's basically impossible this patch is necessary if there's nanosecond precision. There's no way that process interactions take less than a nanosecond, that's just practically impossible. What I'm discovering, though, is that while nanosecond precision is supported it's not necessarily nanosecond-accurate (is that the right term?). If I write a tight loop that creates a file, gets the mtime, and then deletes the file (printing out the timing of each loop iteration) I get (on linux): These operations are thousands of times slower than a nanosecond, yet the timestamp is always showing the same value! Some googling around shows https://stackoverflow.com/questions/14392975/timestamp-accuracy-on-ext4-sub-millsecond which mentions that the kernel caches the current time until the next timer interrupt, which would help explain this behavior. In light of all this, can the comment be updated here? Perhaps something like:
|
That part is surprising to me as well. Thanks for the analysis! I will update the comment. |
|
That also explains, btw, why my tests did not work. |
|
Comment updated. |
|
@bors: r+ |
|
📌 Commit b1fbafd has been approved by |
fix cargo not doing anything when the input and output mtimes are equal That's a problem as the input may have changed in that same second but after the output got generated! Fixes #5918
|
☀️ Test successful - status-appveyor, status-travis |
|
@RalfJung, I know this is from a while ago, but do you know if there's a way to get |
|
@Eh2406 see #5919 (comment). IIRC |
That's a problem as the input may have changed in that same second but after the output got generated!
Fixes #5918