-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add default configs to be consumed by the new schema #949
Conversation
…fined in the C#/VB targets in the MsBuild repo)
@@ -433,5 +433,12 @@ Copyright (c) .NET Foundation. All rights reserved. | |||
<Message Importance="Low" Text="Including @(ReferencePath)" Condition="'%(ReferencePath.ResolvedFrom)' == 'ImplicitlyExpandDesignTimeFacades'" /> | |||
|
|||
</Target> | |||
|
|||
|
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.
Nit: extra whitespace line.
remove or rename them. | ||
--> | ||
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' "/> | ||
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' "/> |
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.
Are we arranging to have this inserted to VS at the same time as the project system supports it?
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.
Without the defaults though the project system will fail miserably so they need to go in together. So we could split this up into two stages, first adding the 2 new defaults and leaving the workaround in place and then a second one to remove the workaround. But that would only help if the SDK changes go in first.
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.
Will this break VS 2017 RTM? What if a user takes a new SDK, but they didn't update their VS?
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 know this is a loaded topic, but there is no currently supported way to do that. You can set MSBuildSdksPath or overwrite the SDK and we've been doing some of that to bootstrap dogfooding while vNext VS branch gets off the ground, but until SDK acquisition is a thing, the SDK used by an unhacked VS is what comes with it and would therefore have a matching project system.
We should investigate how this impacts VS for Mac and VS Code, though.
Instead of making a hard switch, can we have the project system light up on whether Configurations or Platforms are non-empty and leave these matching, innocuous stubs and the capability in place for any consuming project system / IDE that doesn't yet know about the new way?
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 was thinking of ways of working around this and while we can leave the conditioned statements the capability is exclusive. If both capabilities are defined things go very bad. I do have one other idea, I'll sync up offline and circle back on the thread.
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.
Talked offline with Nick and tried another approach here. I'm moving the capability workaround to the design time targets and leaving the old workaround in place. This makes it so that things can flow smoothly in the short run (since this change requires coordination across 4 repos).
#1001 tracks the removal of the workaround.
…gn time targets) and add the InferredFromUsage workaround back.
This change will now come from the sdk. dotnet/sdk#949
Add defaults using the new Configs and Platforms properties.
Note: This depends on dotnet/project-system#1591 which in turns depends on a CPS insertion happening sometime this week.