Skip to content

Import Microsoft.VsSDK.targets conditionally#43343

Merged
JoeRobich merged 2 commits intomasterfrom
dev/tmat/ic-fix
Apr 14, 2020
Merged

Import Microsoft.VsSDK.targets conditionally#43343
JoeRobich merged 2 commits intomasterfrom
dev/tmat/ic-fix

Conversation

@tmat
Copy link
Member

@tmat tmat commented Apr 14, 2020

VSToolsPath might not be initialized during restore.

VSToolsPath might not be initialized during restore.
@tmat tmat requested a review from a team as a code owner April 14, 2020 17:57

<Import Project="$(VSToolsPath)\vssdk\Microsoft.VsSDK.targets"/>
<Import Project="$(VSToolsPath)\vssdk\Microsoft.VsSDK.targets"
Condition="Exists('$(VSToolsPath)\vssdk\Microsoft.VsSDK.targets') and '$(DesignTimeBuild)' != 'true' and '$(BuildingForLiveUnitTesting)' != 'true'"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😕 Why not just the Exists check?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to do any of the work that is implemented in the targets during design time build.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect the VSSDK targets themselves to avoid doing unnecessary work during DTB. Is this not the case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, they do not. Microsoft.VsSDK.targets add following unconditionally:

    <PrepareForRunDependsOn>
      $(PrepareForRunDependsOn);
      GeneratePkgDef;
      CopyPkgDef;
      CreateVsixContainer;
      DeployVsixExtensionFiles;
      CopyVsixManifestFile;
      CopyVsixExtensionFiles;
    </PrepareForRunDependsOn>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DTB does not do a PrepareForRun.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please tell me DTB does not do PrepareForRun. That would be bad.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IDK, doesn't it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VSSDK is doing something during design time build: https://dev.azure.com/devdiv/DevDiv/_workitems/edit/365685?fullScreen=false

Copy link
Member Author

@tmat tmat Apr 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. DTB doesn't run PrepareForRun, only PrepareForBuild. I guess there are other ways VSSDK is hooking into the build.

@JoeRobich JoeRobich merged commit e0cd73a into master Apr 14, 2020
@ghost ghost added this to the Next milestone Apr 14, 2020
@tmat tmat deleted the dev/tmat/ic-fix branch April 16, 2020 18:08
@jasonmalinowski
Copy link
Member

@tmat Is there a bug we need to file against the SDK here?

@sharwell
Copy link
Contributor

@jasonmalinowski No bug. I'll send a PR to remove the unnecessary conditions here.

@sharwell
Copy link
Contributor

@tmat @jasonmalinowski Sent #43444

@sharwell sharwell modified the milestones: Next, temp, 16.7.P1 Apr 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants