-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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 CoreLib fast up-to-date check #69240
Fix CoreLib fast up-to-date check #69240
Conversation
Fixes dotnet#68698 CoreLib's fast up-to-date check was broken because the output pdb was moved into a different folder. Changing this to use a copy task instead and consolidate the two targets into one. I couldn't find a publicly exposed property that allows to change the pdb's output path, hence I decided to copy.
Tagging subscribers to this area: @hoyosjs Issue DetailsFixes #68698 CoreLib's fast up-to-date check was broken because the output pdb was I couldn't find a publicly exposed property that allows to change the
|
</Project> |
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.
Please just ignore this change. I originally had a change in that file and then reverted it in the GH UI. Don't want to spin another build for this...
Inputs="@(_DebugSymbolToMove)" | ||
Outputs="@(_DebugSymbolToMove->Metadata('Destination'))" | ||
Condition="'$(_DebugSymbolsProduced)' == 'true'"> | ||
<Copy SourceFiles="@(_DebugSymbolToMove)" |
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.
Any particular reason we're avoiding SkipUnchangedFiles
?
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.
That setting isn't useful when you leverage Target Inputs and Outputs. The documentation actually warns on using it: https://docs.microsoft.com/en-us/visualstudio/msbuild/copy-task?view=vs-2022.
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.
LGTM aside from small comment
The two test failures are known and got already disabled. I pinged @jakobbotsch offline regarding the msbuild failure as I'm unsure if it's a known issue. |
Condition="'%(_AllPdbs.FolderName)' == 'aotsdk'" /> | ||
|
||
AfterTargets="CopyFilesToOutputDirectory" | ||
DependsOnTargets="_CheckForCompileOutputs" |
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.
Is there any non-internal target that we can hang off of? Like this target is implied by CopyFilesToOutputDirectory
. Is the AfterTargets here not enough?
@@ -9,7 +9,7 @@ | |||
<TargetFramework>$(NetCoreAppCurrent)</TargetFramework> | |||
|
|||
<!-- Force System.Private.CoreLib.dll into a special IL output directory --> | |||
<OutputPath>$(RuntimeBinDir)/IL/</OutputPath> | |||
<OutputPath>$(RuntimeBinDir)IL\</OutputPath> |
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.
Don't we usually normalize or use linux path separators?
Fixes #68698
CoreLib's fast up-to-date check was broken because the output pdb was
moved into a different folder. Changing this to use a copy task instead
and consolidate the two targets into one.
I couldn't find a publicly exposed property that allows to change the
pdb's output path, hence I decided to copy.