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

Update corefx assemblies to .NET 5.0 generation #49789

Merged
merged 6 commits into from
Dec 9, 2020

Conversation

AArnott
Copy link
Contributor

@AArnott AArnott commented Dec 4, 2020

@AArnott AArnott requested a review from a team as a code owner December 4, 2020 16:29
@AArnott AArnott marked this pull request as draft December 4, 2020 16:36
@AArnott AArnott force-pushed the dev/andarno/corefxupdate branch from e4dc872 to fbf8bda Compare December 4, 2020 16:51
@AArnott AArnott marked this pull request as ready for review December 4, 2020 17:00
@AArnott AArnott requested a review from a team as a code owner December 8, 2020 03:11
Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

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

I can't speak to the wisdom of upgrading these packages, but the PR itself seems fine. 😄

@RikkiGibson RikkiGibson self-assigned this Dec 8, 2020
@RikkiGibson
Copy link
Contributor

I'm going to try and test insert this to VS (probably on top of the changes in Andrew's PR to VS) and verify the best I can that everything works together before merging here.

Copy link
Contributor

@jmarolf jmarolf left a comment

Choose a reason for hiding this comment

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

I am ok with this change as long as a test insertion passes

@AArnott
Copy link
Contributor Author

AArnott commented Dec 8, 2020

@RikkiGibson can you share a link to your test PR so I can watch as well?

@RikkiGibson
Copy link
Contributor

I will post the link here as soon as it's created. Signed builds have been finicky this afternoon (seems like network issues?) so I haven't been able to create a test insertion for this yet.

@RikkiGibson
Copy link
Contributor

https://devdiv.visualstudio.com/DevDiv/_git/VS/pullrequest/292063

We still may want to merge your dev branch into this to fully validate things. Feel free to do so if you have some time.

@AArnott
Copy link
Contributor Author

AArnott commented Dec 9, 2020

As expected, that insertion PR failed to build. I've merged my branch into it and expect better things soon. :)

@AArnott
Copy link
Contributor Author

AArnott commented Dec 9, 2020

The test insertion passed.

@AArnott
Copy link
Contributor Author

AArnott commented Dec 9, 2020

@RikkiGibson did you want to validate anything beyond the PR checks?

@RikkiGibson
Copy link
Contributor

Nope, I think we are ready to merge here. Would it be acceptable for me to squash merge this PR?

My plan for this is:

  • we will merge this and produce an official build for it.
  • We will produce an auto-insertion for it that we will not merge.
  • We will give you the commit SHA for the content of the auto insertion, which you will cherry pick into your VS PR and you can merge when ready.

Does that work for you?

@RikkiGibson
Copy link
Contributor

BTW, we seem to be experiencing a publishing issue with our official builds today, so the official build for this may be delayed. /cc @dibarbet

@AArnott
Copy link
Contributor Author

AArnott commented Dec 9, 2020

@RikkiGibson squash merge of this PR is fine. I definitely want to merge the PR going into VS though as those commits are carefully curated.

I'd rather merge my PR into VS right away and have your insertion follow-up and succeed on its own. Since your insertion will appear to 'downgrade' the nuget packages that you optimize, the bot that resolves conflicts will pick the wrong one and I have to manually fix it. But if my PR goes in first (or your insertion PR targets my PR's source branch instead of main) I believe the right thing will happen.
My PR by itself doesn't trigger any RPS regressions, so loss of IBC data from the Unsafe assembly evidently isn't too significant. And since that IBC data will be back with your next insertion, it doesn't seem like they have to go in together.

@RikkiGibson
Copy link
Contributor

I don't think I see a problem with merging your PR to VS standalone, but would like to double check with @tmat as he expressed interest in ensuring that we insert Roslyn at the same time as your change.

@AArnott
Copy link
Contributor Author

AArnott commented Dec 9, 2020

In the meantime, shall we complete this PR?

@RikkiGibson
Copy link
Contributor

Sounds good to me.

Note @dibarbet that if we need to try and insert a version from before this was merged for some reason, we can just look through the master-vs-deps PR history to find the suitable commit we want to build.

@RikkiGibson RikkiGibson merged commit 36d544f into master Dec 9, 2020
@ghost ghost added this to the Next milestone Dec 9, 2020
@RikkiGibson RikkiGibson deleted the dev/andarno/corefxupdate branch December 9, 2020 16:21
@dibarbet dibarbet modified the milestones: Next, 16.9.P3 Dec 19, 2020
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.

7 participants