[Xamarin.Android.Build.Tasks] CopyIfChanged use last "write" time #1968
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
From the "obvious question is obvious" department: there are currently 7 uses of $ git grep File.GetLastAccessTimeUtc
src/Xamarin.Android.Build.Tasks/Tasks/CopyAndConvertResources.cs: var dstmodifiedDate = File.Exists (destfilename) ? File.GetLastAccessTimeUtc (destfilename) : DateTime.MinValue;
src/Xamarin.Android.Build.Tasks/Tasks/CopyAndConvertResources.cs: var dstmodifiedDate = File.Exists (destfilename) ? File.GetLastAccessTimeUtc (destfilename) : DateTime.MinValue;
src/Xamarin.Android.Build.Tasks/Tasks/CopyIfChanged.cs: var dstmodifiedDate = File.Exists (dest) ? File.GetLastAccessTimeUtc (dest) : srcmodifiedDate;
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest.cs: var lastTime = File.GetLastAccessTimeUtc (pdbToMdbPath);
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest.cs: File.GetLastAccessTimeUtc (pdbToMdbPath),
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest.cs: var lastTime = File.GetLastAccessTimeUtc (pdbToMdbPath);
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest.cs: File.GetLastAccessTimeUtc (pdbToMdbPath),Are these also problematic? I suspect that they are. |
|
The build errors are
looks like that is caused by
Hmm |
|
build |
dellis1972
approved these changes
Jul 17, 2018
|
Thats a serious number of typo's :( |
|
Arg conflicts... Fixing it. |
Fixes: dotnet#1962 Since 0adf1ae, I believe that resource changes on macOS have been flaky--sometimes working, sometimes not. They have been consistenly working on Windows, however... After writing the simplest test I could, I found that my machine on High Sierra could *sometimes* reproduce what our QA team was seeing. I had to add the `[Repeat]` NUnit attribute before I could get the failure to occur regularly. After debugging for quite some time, I noticed a mistake in the `CopyIfChanged` task. When comparing timestamps, it looks like it is using the last *access* time of the destination file instead of the last *write* time! I think this is likely a typo (unintended), since switching the call to `GetLastWriteTimeUtc` fixes the problem. Keeping the test is a good idea, because it seems a bit scary we didn't catch this yet...
I suspect that none of these intentional.
jonathanpeppers
force-pushed
the
copyifchanged-facepalm
branch
from
cb6816a
to
a2eac94
Compare
jonpryor
pushed a commit
that referenced
this pull request
Jul 17, 2018
…1968) Fixes: #1962 Since 0adf1ae, I believe that resource changes on macOS have been flaky: sometimes working, sometimes not. They have been consistently working on Windows, however... After writing the simplest test I could, I found that my machine on High Sierra could *sometimes* reproduce what our QA team was seeing. I had to add the `[Repeat]` NUnit attribute before I could get the failure to occur regularly. After debugging for quite some time, I noticed a mistake in the `<CopyIfChanged/>` task: when comparing timestamps, it looks like it is using the last *access* time of the destination file instead of the last *write* time! I think this is likely a typo (unintended), since switching the call to `File.GetLastWriteTimeUtc()` fixes the problem. Keeping the test is a good idea, because it seems a bit scary we didn't catch this yet. With that discovery, we audited the rest of the codebase and found that we were using `File.GetLastAccessTimeUtc()` in several places. We have deemed these *all* to be accidental, and have replaced all occurrences of `File.GetLastAccessTimeUtc()` with `File.GetLastWriteTimeUtc()`.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #1962
Since 0adf1ae, I believe that resource changes on macOS have been
flaky--sometimes working, sometimes not. They have been consistenly
working on Windows, however...
After writing the simplest test I could, I found that my machine on
High Sierra could sometimes reproduce what our QA team was seeing. I
had to add the
[Repeat]NUnit attribute before I could get thefailure to occur regularly.
After debugging for quite some time, I noticed a mistake in the
CopyIfChangedtask. When comparing timestamps, it looks like it isusing the last access time of the destination file instead of the
last write time!
I think this is likely a typo (unintended), since switching the call
to
GetLastWriteTimeUtcfixes the problem. Keeping the test is a goodidea, because it seems a bit scary we didn't catch this yet...