-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Log an error if MSBuildProjectExtensionsPath is modified after it is used #3059
Conversation
--> | ||
<Error Condition=" '$(_InitialMSBuildProjectExtensionsPath)' != '' And '$(MSBuildProjectExtensionsPath)' != '$(_InitialMSBuildProjectExtensionsPath)' " | ||
Code="MSB3540" | ||
Text="The value of the property "MSBuildProjectExtensionsPath" 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." |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
`
There was a problem hiding this comment.
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.
src/Tasks/Microsoft.Common.props
Outdated
<!-- | ||
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. |
There was a problem hiding this comment.
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.
Here's what we decided: MSBuild will add a warning to cover the case where Here's my stab at a warning message:
|
@AndyGerlicher @rainersigwald @cdmihai @dsplaisted this is ready for review |
src/Tasks/Resources/Strings.resx
Outdated
@@ -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 no reuse |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: do not reuse
<!-- | ||
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. |
There was a problem hiding this comment.
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.
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) | ||
|
||
Similar to the error above, there are cases when users set $(BaseIntermediateOutputPath) in the body of their project and things build but only by coincedence. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: coincidence
@dotnet-bot test OSX10.13 Build for CoreCLR please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add any test coverage for this?
--> | ||
<Error Condition=" '$(_InitialMSBuildProjectExtensionsPath)' != '' And '$(MSBuildProjectExtensionsPath)' != '$(_InitialMSBuildProjectExtensionsPath)' " | ||
Code="MSB3540" | ||
Text="The value of the property "MSBuildProjectExtensionsPath" 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" |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
--> | ||
<Warning Condition=" '$(EnableBaseIntermediateOutputPathMismatchWarning)' == 'true' And '$(_InitialBaseIntermediateOutputPath)' != '$(BaseIntermediateOutputPath)' And '$(BaseIntermediateOutputPath)' != '$(MSBuildProjectExtensionsPath)' " | ||
Code="MSB3539" | ||
Text="The value of the property "BaseIntermediateOutputPath" 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 "MSBuildProjectExtensionsPath" 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" |
There was a problem hiding this comment.
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).
Done |
@dotnet-bot |
If
MSBuildProjectExtensionsPath
is set in a project like this:The value used by the top implicit import is different then the value used by the bottom implicit import. Since the
MSBuildProjectExtensionsPath
contains imports generated by NuGet, this behavior can cause some obscure build errors.This change stores the value used by
Microsoft.Common.props
and logs an error if it changed later on.