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 1 commit
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
10 changes: 10 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,16 @@ 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 non-deterministic behavior and builds can fail for obscure reasons.
Copy link
Member

Choose a reason for hiding this comment

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

pedantry: it's not nondeterministic, just confusing.

-->
<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."
Copy link
Contributor

Choose a reason for hiding this comment

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

Change looks good, but the error message might be hard to be actionable. Do we have a docs page with Directory.Build.props explained where we could send people?

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 agree, we thought about making a whole page for this but we could also do an aka.ms link to https://docs.microsoft.com/en-us/visualstudio/msbuild/customize-your-build#directorybuildprops-example

Thoughts?

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 say make a page for the new MSB3540.

@Mikejo5000 do you have thoughts? We're introducing a new error to catch a subtle user error, and we'd like to direct anyone who encounters it to an explanation of the right way to do things. The options proposed so far are: just be terse and rely on search to find the fix, fwlink to a semi-general "how to extend your build" page, and fwlink to a new docs page that describes the error and its fix and cross-links to the general page.

Copy link
Member

Choose a reason for hiding this comment

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

We may add a warning in NuGet (see PR NuGet/NuGet.Client#2056) that should probably link to the same thing. So it wouldn't necessarily be just for MSB3540.

Current proposed text of the NuGet warning:

The BaseIntermediateOutputPath ('{0}') did not match the MSBuildProjectExtensionsPath ('{1}'), which is where the Restore output will go. If you want the Restore output to go to the BaseIntermediateOutputPath, you must set this property before the common .props are imported (for example, in a Directory.Build.props file).

`

Copy link

Choose a reason for hiding this comment

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

@rainersigwald I think making an error page is probably the best choice. Historically, these pages don't get rated very high, but that's often because multiple scenarios can cause the error and multiple possible fixes are involved. The scenario/fix sounds pretty well-known here, so you likely won't have that issue.

/>
</Target>

<!--
Expand Down
7 changes: 7 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 @@ -58,6 +58,13 @@ 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>

<!--
Save the value of MSBuildProjectExtensionsPath to verify during a build that it was not changed 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 non-deterministic behavior
and builds can fail for obscure reasons.
Copy link
Member

Choose a reason for hiding this comment

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

I vote put the comment in only one place since it's wordy and almost duplicated.

-->
<_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 // MSB3540 is used in Microsoft.Common.CurrentVersion.targets to log an error, do no 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