search

microsoft / MSBuildSdks

MSBuild project SDKs
MIT License
459 stars 81 forks source link

NoTargets sdk import discrepancy #443

Open dragosv opened 1 year ago

dragosv commented 1 year ago

Currently the Microsoft.NET.Sdk targets are imported if '$(CommonTargetsPath)' == '' while the props are when '$(MicrosoftCommonPropsHasBeenImported)' != 'true'. This is even more confusing given that CommonTargetsPath is an internal Microsoft.Common.targets property that gets set when Microsoft.Common.targets gets executed. Shouldn't also use the NoTargets Sdk.targets also use MicrosoftCommonPropsHasBeenImported?

jeffkl commented 1 year ago

There are two ways users can declare NoTargets:

<Project Sdk="Microsoft.Build.NoTargets/1.0.0">
 ...
</Project>
<Project>
  <!-- Users can declare any property or import before Sdk.props -->

  <Import Project="Sdk.props" Sdk="Microsoft.Build.NoTargets" Version="1.0.0" />

  <!-- Users can declare any property or import after Sdk.props -->
 ...
  <!-- Users can declare any property or import before Sdk.targets -->

  <Import Project="Sdk.targets" Sdk="Microsoft.Build.NoTargets" Version="1.0.0" />

  <!-- Users can declare any property or import after Sdk.targets -->
</Project>

Given the second example, NoTargets is trying to detect if the user already imported Sdk.props from Microsoft.NET.Sdk or Microsoft.Common.props. The best way to detect that is the MicrosoftCommonPropsHasBeenImported property at the moment. We could possibly change it to '$(UsingMicrosoftNETSdk)' != 'true'. I'm open to changing it if it doesn't break any existing behavior.

For the Sdk.targets import, its essentially the same thing, a detection if the user already imported the common targets from the .NET SDK.

dragosv commented 1 year ago

Could we then have the same condition for both imports? The condition for importing the NetSdk targets is '$(CommonTargetsPath)' == '', while the props is based on MicrosoftCommonPropsHasBeenImported.

jeffkl commented 1 year ago

Unfortunately, no. CommonTargetsPath is set by import Microsoft.Common.props and there's no single property that says you need to import the props at the top and the targets at the bottom. In the case where the user may have imported one or both, we need two different properties to know which to import for them. Is this causing problems for you in a scenario you can share?