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.

Sign up for GitHub

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

Jump to bottom

Log an error if MSBuildProjectExtensionsPath is modified after it is used #3059

Merged
merged 6 commits into from
Mar 19, 2018
Merged

Log an error if MSBuildProjectExtensionsPath is modified after it is used #3059

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2c1e4ea
Log an error if MSBuildProjectExtensionsPath is modified after it is …
jeffkl Mar 7, 2018
7397e1b
Add warning for BaseIntermediateOutputPath changing
jeffkl Mar 13, 2018
5b8e22f
Address comments
jeffkl Mar 13, 2018
835980b
Add ErrorIfChangedInBodyOfProject test
jeffkl Mar 19, 2018
ef58539
Unit test for BaseIntermediateOutputPath
jeffkl Mar 19, 2018
4b5f6c3
Migrate other test cases to Shouldly
jeffkl Mar 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions src/Tasks/Microsoft.Common.CurrentVersion.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -766,6 +766,31 @@ Copyright (C) Microsoft Corporation. All rights reserved.
<PropertyGroup Condition="'$(Prefer32Bit)' == 'true' and ('$(PlatformTarget)' == 'AnyCPU' or '$(PlatformTarget)' == '') and '$(PlatformTargetAsMSBuildArchitectureExplicitlySet)' != 'true'">
<PlatformTargetAsMSBuildArchitecture>x86</PlatformTargetAsMSBuildArchitecture>
</PropertyGroup>

<!--
Log an error if the user set MSBuildProjectExtensionsPath in the body of a project. In an SDK style project
if you set a value in the body, the value is not used by the top implicit import but is used by the bottom.
This can lead to confusing behavior and builds can fail for obscure reasons.
-->
<Error Condition=" '$(_InitialMSBuildProjectExtensionsPath)' != '' And '$(MSBuildProjectExtensionsPath)' != '$(_InitialMSBuildProjectExtensionsPath)' "
Code="MSB3540"
Text="The value of the property &quot;MSBuildProjectExtensionsPath&quot; was modified after it was used by MSBuild which can lead to unexpected build results. To set this property, you must do so before Microsoft.Common.props is imported, for example by using Directory.Build.props. For more information, please visit https://go.microsoft.com/fwlink/?linkid=869650"
Copy link
Member

Choose a reason for hiding this comment

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

Consider using an aka.ms link instead of a fwlink. That way you can choose a URL that's more type-able and remember-able.

/>

<!--
Log a warning if:
1. $(EnableBaseIntermediateOutputPathMismatchWarning) is 'true'
2. $(BaseIntermediateOutputPath) was set in the body of a project after its default value was set in Microsoft.Common.props
3. $(BaseIntermediateOutputPath) is not the same as $(MSBuildProjectExtensionsPath)
Copy link
Member

Choose a reason for hiding this comment

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

Consider disabling the warning if MSBuildProjectExtensionPath was set when the common .props were evaluated. That might be a case where we can assume that the different values were intentional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you'd get the error that MSBuildProjectExtensionPath was modified anyway right?

Copy link
Member

Choose a reason for hiding this comment

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

The scenario I'm thinking of is that you set MSBuildExtensionsPath in Directory.Build.props, but you set BaseIntermediateOutputPath to a different value in the body of your project. Perhaps that's too convoluted to care about, but I think we can infer in that situation that you intended for the paths to be different.


Similar to the error above, there are cases when users set $(BaseIntermediateOutputPath) in the body of their project and things build but only by coincidence.
MSBuild does not know if $(BaseIntermediateOutputPath) changing would cause problems so tools like NuGet must set $(EnableBaseIntermediateOutputPathMismatchWarning)
to 'true'.
-->
<Warning Condition=" '$(EnableBaseIntermediateOutputPathMismatchWarning)' == 'true' And '$(_InitialBaseIntermediateOutputPath)' != '$(BaseIntermediateOutputPath)' And '$(BaseIntermediateOutputPath)' != '$(MSBuildProjectExtensionsPath)' "
Code="MSB3539"
Text="The value of the property &quot;BaseIntermediateOutputPath&quot; was modified after it was used by MSBuild which can lead to unexpected build results. Tools such as NuGet will write outputs to the path specified by the &quot;MSBuildProjectExtensionsPath&quot; instead. To set this property, you must do so before Microsoft.Common.props is imported, for example by using Directory.Build.props. For more information, please visit https://go.microsoft.com/fwlink/?linkid=869650"
Copy link
Member

Choose a reason for hiding this comment

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

I'd try rewording this message. The only unexpected build results that I think we're aware of (once we get these changes in) is that output might be written to a path you don't expect. So I'd avoid being unnecessarily vague.

I'd suggest something like this:

The BaseIntermediateOutputPath ('{0}') did not match the MSBuildProjectExtensionsPath ('{1}'). Tools such as NuGet will write output to the MSBuildProjectExtensionsPath. If you want the output to go to the BaseIntermediateOutputPath, you must set this property before Microsoft.Common.props is imported (for example, in a Directory.Build.props file). See [link] for more info.

I'd also suggest using an aka.ms link here (the same one as above).

/>
</Target>

<!--
Expand Down
3 changes: 3 additions & 0 deletions src/Tasks/Microsoft.Common.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ Copyright (C) Microsoft Corporation. All rights reserved.
-->
<BaseIntermediateOutputPath Condition="'$(BaseIntermediateOutputPath)'=='' ">obj\</BaseIntermediateOutputPath>
<BaseIntermediateOutputPath Condition="!HasTrailingSlash('$(BaseIntermediateOutputPath)')">$(BaseIntermediateOutputPath)\</BaseIntermediateOutputPath>
<_InitialBaseIntermediateOutputPath>$(BaseIntermediateOutputPath)</_InitialBaseIntermediateOutputPath>

<MSBuildProjectExtensionsPath Condition="'$(MSBuildProjectExtensionsPath)' == '' ">$(BaseIntermediateOutputPath)</MSBuildProjectExtensionsPath>
<!--
Import paths that are relative default to be relative to the importing file. However, since MSBuildExtensionsPath
Expand All @@ -58,6 +60,7 @@ Copyright (C) Microsoft Corporation. All rights reserved.
<MSBuildProjectExtensionsPath Condition="'$([System.IO.Path]::IsPathRooted($(MSBuildProjectExtensionsPath)))' == 'false'">$([System.IO.Path]::Combine('$(MSBuildProjectDirectory)', '$(MSBuildProjectExtensionsPath)'))</MSBuildProjectExtensionsPath>
<MSBuildProjectExtensionsPath Condition="!HasTrailingSlash('$(MSBuildProjectExtensionsPath)')">$(MSBuildProjectExtensionsPath)\</MSBuildProjectExtensionsPath>
<ImportProjectExtensionProps Condition="'$(ImportProjectExtensionProps)' == ''">true</ImportProjectExtensionProps>
<_InitialMSBuildProjectExtensionsPath Condition=" '$(ImportProjectExtensionProps)' == 'true' ">$(MSBuildProjectExtensionsPath)</_InitialMSBuildProjectExtensionsPath>
</PropertyGroup>

<Import Project="$(MSBuildProjectExtensionsPath)$(MSBuildProjectFile).*.props" Condition="'$(ImportProjectExtensionProps)' == 'true' and exists('$(MSBuildProjectExtensionsPath)')" />
Expand Down
2 changes: 1 addition & 1 deletion src/Tasks/Resources/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2681,7 +2681,7 @@
MSB3501 - MSB3510 Task: ReadLinesFromFile
MSB3511 - MSB3520 Task: Message
MSB3521 - MSB3530 Task: Warning
MSB3531 - MSB3540 Task: Error
MSB3531 - MSB3540 Task: Error // MSB3539, MSB3540 is used in Microsoft.Common.CurrentVersion.targets, do not reuse
MSB3541 - MSB3550 Task: FindUnderPath
MSB3551 - MSB3580 Task: GenerateResource // MSB3561, MSB3571 not used but already shipped, do not reuse
MSB3581 - MSB3600 Task: ResolveNonMSBuildProjectOutput
Expand Down