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

Use MSBuildProjectExtensionsPath for RestoreOutputPath #2121

Closed

Conversation

dsplaisted
Copy link

@dsplaisted dsplaisted commented Mar 24, 2018

Replaces #2056 with less refactoring and simplified commit history.

This is part of a solution to dotnet/msbuild#1603. The MSBuildProjectExtensionsPath is where MSBuild will automatically import project .props and .targets files from. This PR changes the RestoreOutputPath to default to MSBuildProjectExtensionsPath instead of BaseIntermediateOutputPath. By default, both of these properties will default to the obj folder, but this change will make the behavior more correct if one of the properties is overridden.

This PR also sets the EnableBaseIntermediateOutputPathMismatchWarning property when appropriate, which enables an MSBuild warning if MSBuildProjectExtensionsPath doesn't match BaseIntermediateOutputPath (added in dotnet/msbuild#3059).

Included in this PR is also a commit that replaces new string[0] with Array.Empty<string>(), which was something I noticed when browsing the code investigating this change.

@nkolev92

Copy link
Contributor

@jainaashish jainaashish left a comment

Choose a reason for hiding this comment

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

looks good, great work!

@nkolev92
Copy link
Member

The test results made no sense, so I kicked them off again.

@@ -18,6 +18,13 @@ public interface IVsProjectRestoreInfo
/// </summary>
string BaseIntermediatePath { get; }

// TODO: Update project-system to add this property, and figure out how to sync the updates
Copy link
Contributor

Choose a reason for hiding this comment

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

As we discussed in the meeting earlier, there are 2 options here:

  1. Use current field BaseIntermediatePath to specify MSBuildProjectExtensionsPath value so that won't change anything in interface but change the meaning of this property.

  2. Although we dont have any dictionary at IVsProjectRestoreInfo but we do have a dictionary at IVsProjectRestoreInfo -> IVsTargetFrameworks -> IVsTargetFrameworkInfo -> IVsProjectProperties which is to provide project properties per TFM. So Project system can add MSBuildProjectExtensionsPath at this level and duplicate it for each TFM.

Both these options aren't ideal, but if I've to choose one then I'll vote for approach#1 since option# 2 exposes more vulnerability.

@rrelyea @nkolev92

@dsplaisted
Copy link
Author

@jainaashish @nkolev92 I've updated the IVsProjectRestoreInfo to remove the additional property I added and redefine the meaning of its BaseIntermediatePath property.

I think you can take it from here

@nkolev92
Copy link
Member

Kicked off the tests again.

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

Successfully merging this pull request may close these issues.

4 participants