Skip to content

Fix parsed RID condition for VS - #44998

Merged
stephentoub merged 1 commit into
dotnet:masterfrom
am11:feature/consolidation/msbuild-properties
Nov 20, 2020
Merged

stephentoub merged 1 commit into
dotnet:masterfrom
am11:feature/consolidation/msbuild-properties

Conversation

@am11

@am11 am11 commented Nov 20, 2020

Copy link
Copy Markdown
Member

@ghost

ghost commented Nov 20, 2020

Copy link
Copy Markdown

Tagging subscribers to this area: @safern
See info in area-owners.md if you want to be subscribed.

Issue Details

cc @stephentoub, @ViktorHofer

Author: am11
Assignees: -
Labels:

area-System.Configuration

Milestone: -

@am11

am11 commented Nov 20, 2020

Copy link
Copy Markdown
Member Author

@stephentoub, could you please confirm if it has fixed the VS loading problem? I am not in front of Windows machine rn. :)

@stephentoub

Copy link
Copy Markdown
Member

It does. Thanks.

How did this get checked in with the VS experience entirely broken?

@am11

am11 commented Nov 20, 2020

Copy link
Copy Markdown
Member Author

Thanks for checking. :)
We have consolidated all rid/os/arch/targetplatform property definitions in Configuration.props in #43804. I was using command line to test on different platforms. I think we don't have anything in CI which validates VS experience.

@stephentoub

Copy link
Copy Markdown
Member

We have consolidated all rid/os/arch/targetplatform property definitions in Configuration.props in #43804. I was using command line to test on different platforms. I think we don't have anything in CI which validates VS experience.

@ViktorHofer, in the future, can we make sure that any PRs like that one which impact VS experience are validated manually before merging? Thanks!

@stephentoub

Copy link
Copy Markdown
Member

Merging to unblock folks working in the repo.

@stephentoub
stephentoub merged commit 8df6fe7 into dotnet:master Nov 20, 2020
@ViktorHofer

Copy link
Copy Markdown
Member

@ViktorHofer, in the future, can we make sure that any PRs like that one which impact VS experience are validated manually before merging? Thanks!

I didn't consider the change to impact VS. Let's come up with a policy and define what kind of changes need additional manual validation.

@ViktorHofer ViktorHofer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Thanks

@am11

am11 commented Nov 20, 2020 •

Copy link
Copy Markdown
Member Author

Thanks. @ViktorHofer, would it make sense that one of the windows leg with VS installed, exercise an additional quick step after the build, to validate the VS well being. Something like:

:: build just one SLN with devenv.exe at the end of a CI job

devenv.exe /Build "Release|x64" ^
  src\libraries\Microsoft.Extensions.DependencyModel\Microsoft.Extensions.DependencyModel.sln

@am11
am11 deleted the feature/consolidation/msbuild-properties branch November 20, 2020 15:53
ShreyasJejurkar added a commit to ShreyasJejurkar/runtime that referenced this pull request Nov 20, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants