Check timestamp of symlink target instead of the symlink itself#648
Merged
rainersigwald merged 9 commits intodotnet:masterfrom Jun 27, 2016
Merged
Conversation
While looking around I noticed some minor problems: swapped expected/actual, redundant precondition checks, comment that didn't match code.
These tests reproduce various flavors of dotnet#322. Update tests for symlinks
Fixes dotnet#322. If the path passed to `NativeMethods.GetLastWriteFileUtcTime` is a symlink, MSBuild will now by default return the last write time of the symlink's *target*, not the symlink itself. The old behavior returned the last write time of the symlink itself, which is wrong because it violates the rule of incremental builds: we should only skip a target if we're confident that running the target would produce the same result we already have. Since running the target will probably read the file (through the symlink), the new behavior is more correct. The escape-hatch environment variable `MSBUILDUSESYMLINKTIMESTAMP` is introduced in case this causes unforeseen regressions. Normally I wouldn't introduce a go-back-to-buggy-behavior flag, but since timestamp up-to-date checks are a deep part of MSBuild infrastructure, we're a bit nervous about changing their behavior entirely.
This method was one of the few in the codebase that looked at a FileInfo to compare write-time stamps instead of calling `NativeMethodsShared.GetLastWriteFileUtcTime`.
Symlink creation on Windows requires admin elevation. We don't currently have a way to conditionally run the test only when elevated, so I'm skipping the tests entirely instead.
| private static extern bool SetFileTime(SafeFileHandle hFile, ref long creationTime, | ||
| ref long lastAccessTime, ref long lastWriteTime); | ||
|
|
||
| private void SimpleSymlinkInputCheck(DateTime symlinkWriteTime, DateTime targetWriteTime, |
Contributor
There was a problem hiding this comment.
I'd rename these to inputWriteTime and inputSymlinkWriteTime. At first the parameter names got me really confused.
For some reason (dating back to 2010), Utilities had its own NativeMethods class that defined a GetLastWriteTimeUtc function. That did the same thing as NativeMethodsShared.GetLastWriteFileUtcTime, in the same way, until I changed the latter. Removed the unnecessary function in favor of the shared one.
Member
Author
|
The internal team tried out a build from 651b523 and feels pretty good about it. |
Contributor
|
Open a new issue about nugetizing Filetracker and say that in the meantime if people want to run the filetracker tests they should copy in the dlls from program files? LGTM, if that's not a flaky failing test. |
Member
Author
|
@cdmihai This one's not about FileTracker. IIRC the only known remaining work I have for this is to update the Copy task's timestamp checks. |
Contributor
|
Whoops, fatal brain fault on my part. :) |
GetLastWriteFileUtcTime handles reading through symlinks and nonexistent input files, so it's more appropriate here. Part of dotnet#322.
Instead of `FileInfo.LastWriteTime`, use `NativeMethodsShared.GetLastWriteFileUtcTime`. This should unify the behavior of GenerateResource's internal incrementality on the common technique used for dotnet#322. I removed some comments about creating a `FileInfo` as a "single call to determine presence + modified time". Conveniently, `GetLastWriteFileUtcTime` does the same thing, with the sentinel value `DateTime.MinValue` representing "file not found".
…p-destination Conflicts: src/XMakeBuildEngine/BackEnd/Components/RequestBuilder/TargetUpToDateChecker.cs Upstream changed to use exception filters, but this codepath calls native methods and doesn't throw on failure, so try/catch blocks shouldn't be necessary here.
Member
Author
|
@AndyGerlicher You mentioned wanting to take another look at this . . . |
Contributor
|
|
Closed
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
See #322. The Microsoft team that reported this is testing a private build based on this code, so this isn't quite ready to go: