Skip to content

Upgrade Arcade to a 5.0 SDK - #5357

Merged
ViktorHofer merged 4 commits into
masterfrom
ViktorHofer-SDK50
May 4, 2020
Merged

Upgrade Arcade to a 5.0 SDK#5357
ViktorHofer merged 4 commits into
masterfrom
ViktorHofer-SDK50

Conversation

@ViktorHofer

@ViktorHofer ViktorHofer commented Apr 27, 2020

Copy link
Copy Markdown
Member

Fixes #5394

To support the net5.0 TFM change we need to update the SDK for a newer NuGet.

To support the net5.0 TFM change we need to update SDK for a newer NuGet.
@ViktorHofer
ViktorHofer requested a review from markwilkie April 27, 2020 18:59
@markwilkie
markwilkie requested review from jaredpar and mmitche April 27, 2020 19:08
@markwilkie

Copy link
Copy Markdown
Contributor

This makes sense to me, but running it by @mmitche and @jaredpar as well as I might be missing something.

@ghost

ghost commented Apr 27, 2020

Copy link
Copy Markdown

Should we move to the latest released SDK?
https://github.com/dotnet/arcade/blob/master/Documentation/Policy/ToolsetPolicy.md#net-sdk--managed-toolset

mentions staying on the latest released preview (which we also haven't done), but also mentions moving to a non-released one should come with approval from tactics.

@ViktorHofer

Copy link
Copy Markdown
Member Author

We can downgrade to a officially released P2 or P3 build. Picked the latest nightly build as it includes some VSTest features that might be useful for #5347 (ie specifying a default filter in the .runsettings file, pointing to a custom dotnet host and not swallowing build output during VSTest).

@ViktorHofer

Copy link
Copy Markdown
Member Author

@swaroop-sridhar can you please take a look at the .deps.json build failures? Looks related to dotnet/aspnetcore#21017 (comment). cc @dougbu

@dougbu

dougbu commented Apr 27, 2020

Copy link
Copy Markdown
Contributor

I suppressed NU5118 warnings because figuring out the root cause was going to take too much time. An actual fix would be great.

@ViktorHofer

Copy link
Copy Markdown
Member Author

ping @swaroop-sridhar

@ghost ghost mentioned this pull request Apr 28, 2020
@mmitche

mmitche commented Apr 28, 2020

Copy link
Copy Markdown
Member

This is sending warning bells that we may have attempted this before and decided against it. Maybe @jaredpar remembers?

@swaroop-sridhar

Copy link
Copy Markdown
Contributor

ping @swaroop-sridhar

I posted a PR for the PublishDepsFilePath setting dotnet/sdk#11462
PublishDepsFilePath is also used by the packing targets, so it does seem related to the issue.

@ViktorHofer

Copy link
Copy Markdown
Member Author

@swaroop-sridhar the failures are still the same with the new SDK.

@ViktorHofer

Copy link
Copy Markdown
Member Author

The error goes away if I comment out all the PackageReferences used during the build.

@ViktorHofer

Copy link
Copy Markdown
Member Author

Submitted a fix to the SDK: dotnet/sdk#11522

@ViktorHofer

Copy link
Copy Markdown
Member Author

@markwilkie this should be ready now. Can we clarify if we are fine upgrading to a preview5 SDK? IMO it shouldn't be an issue.

@dougbu

dougbu commented May 1, 2020

Copy link
Copy Markdown
Contributor

See also the breaks we hit in dotnet/razor#1692 (comment) and dotnet/aspnetcore#21017 (comment). Discussion about the second problem continues in my dotnet/sdk#11527 PR.

@dougbu

dougbu commented May 1, 2020

Copy link
Copy Markdown
Contributor

Oops, forgot about incorrect NU5129 warnings I commented on at dotnet/aspnetcore#21017 (comment) and today at dotnet/aspnetcore#21017 (comment) with responses later from @rrelyea.

@dougbu

dougbu commented May 1, 2020

Copy link
Copy Markdown
Contributor

For the dotnet/aspnetcore#21017 (comment) break, my dotnet/sdk#11527 PR is on hold for further discussion. Repos other than dotnet/aspnetcore that adjust .deps files may need to adjust as I did in dotnet/aspnetcore@3ef5452 i.e. run their rewrite target later or use $(IntermediateDepsFilePath) instead of $(PublishDepsFilePath).

@ViktorHofer

Copy link
Copy Markdown
Member Author

@markwilkie I downgraded to the official preview3 released version which should be less controversial. I want to get this in as it blocks the build promotion.

@markwilkie

Copy link
Copy Markdown
Contributor

Makes sense @ViktorHofer. If @mmitche, @Pilchie and @jaredpar are good with this, I am too. An email head's up to tactics is probably in order too, especially given the noise.

Adding @missymessa as we'll hopefully be able to promote today and it sounds like you need this in soon.

@Pilchie

Pilchie commented May 4, 2020

Copy link
Copy Markdown
Member

Yes, I'm good with this.

@ViktorHofer
ViktorHofer force-pushed the ViktorHofer-SDK50 branch from b1f3295 to b1a0cc9 Compare May 4, 2020 16:43
@ViktorHofer

Copy link
Copy Markdown
Member Author

Approved by Tactics - merging.

@ViktorHofer
ViktorHofer merged commit 17288f0 into master May 4, 2020
@ViktorHofer
ViktorHofer deleted the ViktorHofer-SDK50 branch May 4, 2020 17:27
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.

Could not load file or assembly 'NuGet.Common, Version=5.6.0.2

6 participants