-
Notifications
You must be signed in to change notification settings - Fork 347
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
Condition RestoreInternalTooling on OfficialBuild=true #14974
Conversation
@@ -14,7 +14,7 @@ | |||
|
|||
<!-- Internal tool restore settings --> | |||
<!-- VisualStudio.InsertionManifests.targets: Keep condition in sync with import in AfterSigning.proj. --> | |||
<RestoreInternalTooling Condition="'$(UsingToolVSSDK)' == 'true' and '$(MSBuildRuntimeType)' != 'Core'">true</RestoreInternalTooling> | |||
<RestoreInternalTooling Condition="'$(UsingToolVSSDK)' == 'true' and '$(MSBuildRuntimeType)' != 'Core' and '$(OfficialBuild)' == 'true'">true</RestoreInternalTooling> |
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 will work, but I'm not entirely sure it is correct (since a local dev could want to run these targets). The normal pattern for restoring internal tooling has been to have the target that requires the internal tooling depend on the internal tooling restore target (see _PrepareAcquireVisualStudioOptimizationData
) rather than trying to predict whether it's necessary. Does this pattern not work in this case?
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.
Unfortunately that doesn't work here.
<Import Project="VisualStudio.InsertionManifests.targets" Condition="'$(UsingToolVSSDK)' == 'true' and '$(MSBuildRuntimeType)' != 'Core'" /> VisualStudio.InsertionManifests.targets
- The
GenerateVisualStudioInsertionManifests
target depends on internal tools. That target runs whenever desktop msbuild is used and theUsingToolVSSDK
property is set to true (repos set that in their Versions.props i.e.).
That target in theory runs in public as well as AfterSigning.proj
is always invoked:
arcade/src/Microsoft.DotNet.Arcade.Sdk/tools/Build.proj
Lines 287 to 290 in 4dc2518
<MSBuild Projects="AfterSigning.proj" | |
Properties="@(_CommonProps);_NETCORE_ENGINEERING_TELEMETRY=Sign" | |
Targets="@(_SolutionBuildTargets)" | |
Condition="'@(_SolutionBuildTargets)' != ''"/> |
But back to your question, we can't depend on the RestoreInternalTooling
target directly as AfterSigning.proj is in another project and we should only restore internal tools up-front and not after the build.
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.
FWIW I think we should merge this PR in to fix the regression and then follow-up on a better design.
We might need an opt-in property for the GenerateVisualStudioInsertionManifests
target.
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.
Sounds good
To double check: