Skip to content
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

Issue error or warning if BaseIntermediateOutputPath is modified after it's used to calculate MSBuildProjectExtensionsPath #803

Closed
dsplaisted opened this issue Feb 3, 2017 · 6 comments

Comments

@dsplaisted
Copy link
Member

Filing this in the SDK repo for now, though we may want to do this in MSBuild itself.

If you try to override the BaseIntermediateOutputPath property in a project that uses the <Project Sdk="Microsoft.NET.Sdk"> syntax, you will be setting the property after it's already been used to calculate the MSBuildProjectExtensionsPath property, which is where the auto-generated NuGet .props and .targets files are imported from. So you won't get any imports from NuGet packages.

The solution to this is to set the BaseIntermediateOutputPath in a Directory.Import.props file, which gets evaluated beforehand, or not to use the Sdk attribute on the project and instead have a pair of .props and .targets imports in the project to import from the Sdk, and to set the BaseIntermediateOutputPath before the .props import.

This is likely to be a common issue people run into, and it will not be obvious what is wrong or how to fix it. So we should consider detecting that the BaseIntermediateOutputPath property was modified after it was used to calculate MSBuildProjectExtensionsPath, and generate an error (or at least a warning).

Related issue for us to fix this for the SDK build itself: #802

@dsplaisted
Copy link
Member Author

@rainersigwald @AndyGerlicher What do you folks think about adding a warning or error for this in MSBuild compared to doing it in the .NET SDK?

@srivatsn
Copy link
Contributor

srivatsn commented Feb 3, 2017

I'd filed dotnet/msbuild#1603 in MSBuild about this.

@rainersigwald
Copy link
Member

I don't see a good way to catch this in common targets alone. The usual (admittedly terrible) approach to this sort of check is to set a shadow property (maybe _OriginalBaseIntermediateOutputPath) to the value of the real property at or before the point of first use, and then have a target run to validate that the two properties match. Since the point of first use is in the Sdk, MSBuild can't help there.

In the past, folks have brought up notions like write-once or defaulted properties (I couldn't find an issue for this, so I filed dotnet/msbuild#1673). That could help here but would be a serious language change.

@dsplaisted
Copy link
Member Author

Since the point of first use is in the Sdk

That doesn't seem to be true. I think the first point of use is in Microsoft.Common.props.

Perhaps instead of having a shadow property, we could simply compare against MSBuildProjectExtensionsPath, although we'd need to apply the same path-rooting logic to the intermediate output path before comparing:

<MSBuildProjectExtensionsPath Condition="'$([System.IO.Path]::IsPathRooted($(MSBuildProjectExtensionsPath)))' == 'false'">$([System.IO.Path]::Combine('$(MSBuildProjectDirectory)', '$(MSBuildProjectExtensionsPath)'))</MSBuildProjectExtensionsPath>

@srivatsn srivatsn added the Bug label Feb 6, 2017
@srivatsn srivatsn added this to the 1.1 milestone Feb 6, 2017
dsplaisted added a commit to dsplaisted/sdk that referenced this issue Mar 13, 2017
@dsplaisted dsplaisted modified the milestones: 2.0, 1.1 May 15, 2017
@dsplaisted dsplaisted self-assigned this May 31, 2017
@livarcocc livarcocc modified the milestones: 2.1.0, 2.0.0 Jul 12, 2017
@dsplaisted
Copy link
Member Author

MSBuild has a PR for this: dotnet/msbuild#3059

@dsplaisted
Copy link
Member Author

Fixed with NuGet/NuGet.Client#2131 and dotnet/msbuild#3059

mmitche pushed a commit to mmitche/sdk that referenced this issue Jun 5, 2020
…0190724.11 (dotnet#803)

- Microsoft.AspNetCore.Mvc.Analyzers - 3.0.0-preview8.19374.11
- Microsoft.AspNetCore.Mvc.Api.Analyzers - 3.0.0-preview8.19374.11
- Microsoft.AspNetCore.Analyzers - 3.0.0-preview8.19374.11
- Microsoft.AspNetCore.Components.Analyzers - 3.0.0-preview8.19374.11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants