Skip to content

Don't set a default ProjectAssetsFile path in the SDK #2010

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

Merged
merged 5 commits into from
Apr 19, 2018

Conversation

dsplaisted
Copy link
Member

@dsplaisted dsplaisted commented Mar 1, 2018

Resolves #1486
Resolves #1438, and #1057 (it doesn't enable the scenario, but does fix the SDK's piece)

NuGet sets the ProjectAssetsFile property in the .g.props file it generates, so this property should already be set by the time we would get to the code this PR removes.

TODO: Test whether this affects behavior when restore hasn't occurred.

@livarcocc livarcocc added this to the 2.1.3xx milestone Mar 1, 2018
@dsplaisted dsplaisted force-pushed the dont-set-assets-path branch from dc5b1e5 to 4bb702d Compare March 3, 2018 01:09
@dsplaisted
Copy link
Member Author

@dotnet-bot test OSX10.12 Debug
test OSX10.12 Release

<trans-unit id="AssetsFileNotSet">
<source>ProjectAssetsFile was not set. Run a NuGet package restore to generate this file.</source>
<target state="new">ProjectAssetsFile was not set. Run a NuGet package restore to generate this file.</target>
<note>"ProjectAssetsFile" is the name of a property and parameter to the task, so it should not be translated</note>
Copy link
Contributor

@nguerrera nguerrera Mar 4, 2018

Choose a reason for hiding this comment

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

This is a common enough case, right? e.g. You'd get it using msbuild without /restore with no prior restore?. I don't think we should be mentioning ProjectAssetsFile the variable, but rather the fact that we can't locate the project assets file. This is getting into the implementation details.

@dsplaisted
Copy link
Member Author

I've updated the message per @nguerrera's suggestion. However, we may want to wait until NuGet/NuGet.Client#2056 is ready to merge before making this change. Otherwise, projects which set BaseIntermediateOutputPath after the common .props have been imported will fail to build (whereas with current behavior, they will probably build but may build incorrectly, as the .props from NuGet packages won't be imported).

@tannergooding
Copy link
Member

@dsplaisted, looks like the NuGet.Client change was merged 12 days ago with NuGet/NuGet.Client#2131, can this be rebased and merged as well?

@dsplaisted
Copy link
Member Author

@tannergooding It just got merged into the CLI yesterday: dotnet/cli#9040

So yes, I think this is unblocked now

@dsplaisted dsplaisted force-pushed the dont-set-assets-path branch 3 times, most recently from 61be612 to ea0b7ab Compare April 14, 2018 01:17
@dsplaisted
Copy link
Member Author

@MattGertz for approval

Issues fixed
#1486, #1438, and #1057 (Most of the fix to these issues was in NuGet and MSBuild, this PR is for a comparatively small change in the SDK to align with those changes).

Description of Issue
Inconsistent MSBuild properties are used for the path to NuGet restore output. If the values of these properties differ, builds can be broken (restore writes output to one place, and the build looks elsewhere and can't find it)

Customer Impact
Most commonly, projects which set BaseIntermediateOutputPath after Microsoft.Common.props has been evaluated won't pick up the generated NuGet .props and .targets files, leading to what could be arbitrary build issues that are hard to diagnose the cause.

Risk
Low

Testing
Unit test coverage

@MattGertz
Copy link

@livarcocc Does this fall under the domain of .NET Shiproom? For my part, I would approve this.

@dsplaisted dsplaisted changed the base branch from master to release/2.1.3xx April 17, 2018 21:41
@MattGertz
Copy link

We're on the same page --- this is pending shiproom approval.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants