Skip to content
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

Fix NativeMethodsShared.GetLastWriteFileTimeUtc when the path is a #4059

Merged
merged 1 commit into from Jan 14, 2019

Conversation

@radical
Copy link
Member

commented Jan 10, 2019

.. directory.

GetLastWriteFileTimeUtc depends on the incorrect assumption[1] that
File.GetLastWriteTimeUtc will return DateTime.FromFileTimeUtc(0) if
the path is a directory.

The same thing applies for the Directory equivalent of the same
method.

This breaks File existence checks, for example in
ResolveAssemblyReference when we have a reference like:

<Reference Include="FooBar" />

.. where FooBar is a directory in the current(project) directory.
ResolveAssemblyReference ends up triyng to get assembly information from
the file, because it thinks that FooBar is a file and thus an assembly
path.

@radical radical requested a review from ccastanedaucf Jan 10, 2019
@radical

This comment has been minimized.

Copy link
Member Author

commented Jan 10, 2019

This likely affects perf, but I haven't tested it.

@radical radical requested a review from rainersigwald Jan 10, 2019
@radical radical force-pushed the radical:fix-file-time-check branch from 623a077 to 89b0f75 Jan 10, 2019
@ccastanedaucf

This comment has been minimized.

Copy link
Collaborator

commented Jan 11, 2019

I remember this extra IO call added somewhere around an extra second on Linux vs Windows on a ~15 second build, but I guess there's no way to avoid hitting the disk twice. IO in RAR has been reduced since then (which makes up the majority of these calls), so that difference could be smaller now.

@radical

This comment has been minimized.

Copy link
Member Author

commented Jan 11, 2019

I guess this is similarly assuming that GetFileAttributesEx will return false if the file path is a directory, which is not true (https://docs.microsoft.com/en-us/windows/desktop/api/fileapi/nf-fileapi-getfileattributesexa), so it will need a similar check. Am I correct?

@radical

This comment has been minimized.

Copy link
Member Author

commented Jan 11, 2019

@ccastanedaucf

This comment has been minimized.

Copy link
Collaborator

commented Jan 11, 2019

I'm confused on that last point, since it looks like the first method performs the inverse of that FileExists condition then sets success to false if it's not a directory.

@radical

This comment has been minimized.

Copy link
Member Author

commented Jan 11, 2019

@ccastanedaucf oh, sorry, you are correct. I mixed that up. What should this be changed to? Looking at the file attributes on windows, IIUC, there are multiple attributes that might indicate a file. So, maybe we should just check that it is not a FILE_ATTRIBUTE_DIRECTORY?

@ccastanedaucf

This comment has been minimized.

Copy link
Collaborator

commented Jan 11, 2019

I think just adding && (data.fileAttributes & NativeMethodsShared.FILE_ATTRIBUTE_DIRECTORY) == 0 to the success condition would be enough there, and hopefully nothing is depending on that behavior.

.. directory.

`GetLastWriteFileTimeUtc` depends on the incorrect assumption[1] that
`File.GetLastWriteTimeUtc` will return `DateTime.FromFileTimeUtc(0)` if
the path is a directory.

The same thing applies for the `Directory` equivalent of the same
method.

This breaks File existence checks, for example in
`ResolveAssemblyReference` when we have a reference like:

`<Reference Include="FooBar" />`

.. where `FooBar` is a directory in the current(project) directory.
ResolveAssemblyReference ends up triyng to get assembly information from
the file, because it thinks that `FooBar` is a file and thus an assembly
path.
@radical radical force-pushed the radical:fix-file-time-check branch from 89b0f75 to b2d8099 Jan 14, 2019
Copy link
Contributor

left a comment

LGTM. @ccastanedaucf, want to take a final look?

Copy link
Collaborator

left a comment

Looked over it w/ Mihai earlier, looks good!

@radical radical merged commit 53ad97a into microsoft:master Jan 14, 2019
3 checks passed
3 checks passed
WIP Ready for review
Details
license/cla All CLA requirements met.
Details
msbuild-pr #20190114.1 succeeded
Details
@radical

This comment has been minimized.

Copy link
Member Author

commented Jan 14, 2019

Thanks for the reviews!

@radical radical deleted the radical:fix-file-time-check branch Jan 14, 2019
radical added a commit to radical/msbuild that referenced this pull request Jan 18, 2019
…icrosoft#4059)

.. directory.

`GetLastWriteFileTimeUtc` depends on the incorrect assumption[1] that
`File.GetLastWriteTimeUtc` will return `DateTime.FromFileTimeUtc(0)` if
the path is a directory.

The same thing applies for the `Directory` equivalent of the same
method.

This breaks File existence checks, for example in
`ResolveAssemblyReference` when we have a reference like:

`<Reference Include="FooBar" />`

.. where `FooBar` is a directory in the current(project) directory.
ResolveAssemblyReference ends up triyng to get assembly information from
the file, because it thinks that `FooBar` is a file and thus an assembly
path.

(cherry picked from commit 53ad97a)
radical added a commit to radical/msbuild that referenced this pull request Jan 18, 2019
…icrosoft#4059)

.. directory.

`GetLastWriteFileTimeUtc` depends on the incorrect assumption[1] that
`File.GetLastWriteTimeUtc` will return `DateTime.FromFileTimeUtc(0)` if
the path is a directory.

The same thing applies for the `Directory` equivalent of the same
method.

This breaks File existence checks, for example in
`ResolveAssemblyReference` when we have a reference like:

`<Reference Include="FooBar" />`

.. where `FooBar` is a directory in the current(project) directory.
ResolveAssemblyReference ends up triyng to get assembly information from
the file, because it thinks that `FooBar` is a file and thus an assembly
path.

(cherry picked from commit 53ad97a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.