-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Conversation
FYI @dpodder who appears to be changing same targets in dotnet/buildtools#1427 |
@dpodder The changes look pretty simple to merge. I'll watch for a new version of buildtools and update corefx onto it and merge in the codeOptimization.targets changes into Tools-Override here. |
<OptimizationDataNuGetFeed Condition="'$(OptimizationDataNuGetFeed)'==''">https:%2F%2Fdotnet.myget.org/F/roslyn/api/v3/index.json</OptimizationDataNuGetFeed> | ||
</PropertyGroup> | ||
|
||
<MakeDir Directories="$(OptimizationDataDir)" ContinueOnError="true" /> | ||
<WriteLinesToFile File="$(OptimizationDataProjectJson)" Lines="@(_OptimizationDataJsonLine)" Overwrite="true" /> | ||
<Copy SourceFiles="$(OptimizationDataSourceProject)" DestinationFiles="$(OptimizationDataRestoreTarget)" /> |
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.
Do we actually need to copy it? The only reason I could imagine would be if nuget wasn't smart enough to invalidate the restore when we change the global properties. It's not like this was even doing incremental build all that well since the package name/version weren't in the path.
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.
We don't strictly need to, but it does control where the intermediate outputs go to.
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.
... unless you pass in a property for that as well.
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'll do that in a follow-up. I'm going to merge now to unblock the official builds.
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.
Consider simplifying the restore target if it works correctly on the .msbuild file with only changing global properties.
c491c31
to
032050e
Compare
I rebased on master so that CI won't hit the problem fixed by #18213. |
There were two issues related to my recent change ( #18035 ):
EnableProfileGuidedOptimization
is set totrue
, which doesn't happen in CI builds. I've modified the targets file to use a checked-in MSBuild project instead. An unchanged version of codeOptimization.targets was checked in in order to show the diff (look at the second commit to see just the diff).@weshaggard