-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Make polling use the symbolic link target's LastWriteTime #55664
Changes from all commits
f515710
08233ba
d780995
ac4a845
b4895ad
ebb0326
1164e33
98b737a
75fcf96
144335a
9c50a3a
b2f9bad
d8d143a
c7e28d4
5b85631
5c27cae
19dd9c3
8c1b3a2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,14 @@ | ||
<Project Sdk="Microsoft.NET.Sdk"> | ||
<PropertyGroup> | ||
<TargetFrameworks>netstandard2.0;net461</TargetFrameworks> | ||
<TargetFrameworks>$(NetCoreAppCurrent);netstandard2.0;net461</TargetFrameworks> | ||
</PropertyGroup> | ||
<ItemGroup> | ||
<Compile Include="Microsoft.Extensions.FileProviders.Abstractions.cs" /> | ||
</ItemGroup> | ||
<ItemGroup> | ||
<ProjectReference Include="$(LibrariesProjectRoot)Microsoft.Extensions.Primitives\ref\Microsoft.Extensions.Primitives.csproj" /> | ||
</ItemGroup> | ||
<ItemGroup Condition="'$(TargetFramework)' == '$(NetCoreAppCurrent)'"> | ||
<ProjectReference Include="$(LibrariesProjectRoot)System.Runtime\ref\System.Runtime.csproj" /> | ||
</ItemGroup> | ||
</Project> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System; | ||
using System.Diagnostics; | ||
using System.IO; | ||
|
||
namespace Microsoft.Extensions.FileProviders.Physical | ||
|
@@ -27,5 +28,40 @@ public static bool IsExcluded(FileSystemInfo fileSystemInfo, ExclusionFilters fi | |
|
||
return false; | ||
} | ||
|
||
public static DateTime? GetFileLinkTargetLastWriteTimeUtc(string filePath) | ||
{ | ||
#if NETCOREAPP | ||
var fileInfo = new FileInfo(filePath); | ||
if (fileInfo.Exists) | ||
{ | ||
return GetFileLinkTargetLastWriteTimeUtc(fileInfo); | ||
} | ||
#endif | ||
return null; | ||
} | ||
|
||
// If file is a link and link target exists, return target's LastWriteTimeUtc. | ||
// If file is a link, and link target does not exists, return DateTime.MinValue | ||
// since the link's LastWriteTimeUtc doesn't convey anything for this scenario. | ||
// If file is not a link, return null to inform the caller that file is not a link. | ||
public static DateTime? GetFileLinkTargetLastWriteTimeUtc(FileInfo fileInfo) | ||
{ | ||
#if NETCOREAPP | ||
Debug.Assert(fileInfo.Exists); | ||
if (fileInfo.LinkTarget != null) | ||
{ | ||
FileSystemInfo targetInfo = fileInfo.ResolveLinkTarget(returnFinalTarget: true); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jozkee have you considered keeping all these libs .NET Standard 2.0 and referencing the code that defines @jeffhandley @ericstj how long do we plan to keep targeting There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
At the moment we have no plans for dropping There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, we're keeping ns2.0 and net461 until further notice. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can do a different (and more efficient) approach to get the target's Last write time, see @ericstj's suggestion #55664 (comment). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@jozkee I wanted to know whether it would be possible to make this fix work for all TFMs (6 + NS2 + net461), because currently, it's going to work only for .NET 6. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is, but we can defer fixing this on other TFMs if no one is asking for it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think to fix on older TFMs it needs more than just MS.IO.Redist. I believe the implementation of the IO API you are using here (or would add) would depend on the system-native PALs which we don't consider part of the public API. Having a package depend on those is fragile at the least, and might not even work if the versions of those don't have the exports you need on older frameworks. Adding private copies of the system-native PALs (similar to what System.IO.Ports does) is expensive too. Something to consider around OOB'ing our IO API. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahh, good info @ericstj. So far, we don't have any requests to have this fix apply to previous versions; I'm OK with this being net6.0+ until such requests come in. |
||
if (targetInfo.Exists) | ||
{ | ||
return targetInfo.LastWriteTimeUtc; | ||
} | ||
|
||
return DateTime.MinValue; | ||
} | ||
#endif | ||
|
||
return null; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this unsupported on browser? Is the whole assembly not supported on browser?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this project references System.IO.FileSystem.Watcher and that is not supported in browser:
runtime/src/libraries/System.IO.FileSystem.Watcher/Directory.Build.props
Line 6 in 7aaffcb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, so then should we add a runtime assembly for Browser that throws PNSE?
cc: @marek-safar @steveisok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/cc @lewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes which is what this setting does
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This setting doesn't generate a PNSE automatically, this setting is just to add an assembly attribute for the platform analyzer. Unless something changed since last time I did it, in order to generate PNSE we need to add a
-Browser
tfm and then set the flag to generate a pnse when TargetsBrowser == true. So then it seems like we should do that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this, I've addressed @safern's comment in 75fcf96, please let me know if something else (related to mark the project as unsupported in Browser) needs to be done.