-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fix dotnet-run-file up-to-date checks across symlinks #52064
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 dotnet-run-file up-to-date checks across symlinks #52064
Conversation
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.
Pull request overview
This PR fixes an issue where dotnet run with file-based programs incorrectly treated files as up-to-date when they were modified through symbolic links. The fix ensures that timestamp checks resolve symlinks to their final targets, so modifications to the target file are properly detected.
Key changes:
- Added
ResolveLinkTargethelper to resolve symlinks before checking file modification times - Applied the fix to both entry point files and implicit build files
- Added comprehensive test coverage for symlink scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Cli/dotnet/Commands/Run/VirtualProjectBuildingCommand.cs | Added symlink resolution logic to up-to-date checks, resolving both entry point and implicit build files to their final targets before comparing timestamps |
| test/dotnet.Tests/CommandTests/Run/RunFileTests.cs | Added four new test cases covering symlink scenarios: basic up-to-date checks, CSC-only optimization, CSC-after-MSBuild optimization, and EntryPointFilePath behavior with symlinks |
|
|
||
| return false; | ||
|
|
||
| static FileSystemInfo ResolveLinkTarget(FileSystemInfo fileSystemInfo) |
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.
Perhaps ResolveLinkTargetOrSelf, to differentiate from FileSystemInfo.ResolveLinkTarget?
|
|
||
| static FileSystemInfo ResolveLinkTarget(FileSystemInfo fileSystemInfo) | ||
| { | ||
| return fileSystemInfo.ResolveLinkTarget(returnFinalTarget: true) ?? fileSystemInfo; |
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.
Might be good to exercise the returnFinalTarget behavior, e.g. symlink2 links to symlink1 links to original file, ensuring that symlink2 has correct up to date behavior with respect to original file.
|
What is the behavior of symlinks with respect to implicit build files? Is it expected that different implicit build files would be brought in, when running I would personally find it a bit strange if the different invocations used different implicit build files. My hope would be that we would resolve the symlink very early on in this process, and consistently use the same "implicit build files" whether we are building the symlink or the original thing. It might also be worth checking what Possibly the behavior I am asking for is already present and I missed it. Apologies if that is the case. |
If the project is symlinked, it's as if it was copied (so dir.build.props are taken from the symlink's directory, not its target; i.e., different build files are brought in for |
| /// See <see href="https://github.com/dotnet/sdk/pull/52064#issuecomment-3628958688"/>. | ||
| /// </summary> | ||
| [Fact] | ||
| public void DirectoryBuildProps_SymbolicLink() |
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.
Thank you for adding the test. I think no need to change behavior in this PR. We can see if anyone complains about this behavior during the upcoming dev cycle, and decide at that point whether there is motivation to make a change.
|
/ba-g known failures |
Fixes #52063.