Skip to content

Remove usages of private CoreCLR and CoreFX packages from SharedFramework SDK. - #4708

Merged
MattGal merged 4 commits into
dotnet:masterfrom
jkoritzinsky:no-private-packages-sharedframework-sdk
Jan 30, 2020
Merged

Remove usages of private CoreCLR and CoreFX packages from SharedFramework SDK.#4708
MattGal merged 4 commits into
dotnet:masterfrom
jkoritzinsky:no-private-packages-sharedframework-sdk

Conversation

@jkoritzinsky

Copy link
Copy Markdown
Member

This moves us closer to being able to remove the old private transport packages from pre-consolidation from dotnet/runtime.

cc: @dagood

@dagood dagood 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.

Looks good, thanks!

/cc @NikolaMilosavljevic FYI

@dagood

dagood commented Jan 29, 2020

Copy link
Copy Markdown
Member

Have you tried this on https://github.com/dotnet/windowsdesktop (and/or dotnet/runtime)?

@jkoritzinsky

Copy link
Copy Markdown
Member Author

I've validated with dotnet/windows-desktop. There is some reactionary work needed to define a few properties (MicrosoftNETCoreAppInternalPackage and MicrosoftNETCoreAppPackage) and to add a dependency on Microsoft.NETCore.App.Internal, but other than that it works.

@dagood dagood 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.

Hmm, looks ok, but I preferred using the runtime pack. (Because it's what we actually ship--.Internal is a nonshipping test compatibility/holdover package that ideally we should delete at some point.) Did you run into problems?

@jkoritzinsky

Copy link
Copy Markdown
Member Author

There's no rid-agnostic version of the runtime pack, so we don't have a package that we can directly add a reference to that will restore the correct thing on all platforms. I also wanted to use the runtime pack, but I ended up having to fall back to using .Internal since it was the only thing with something I could reference.

@dagood

dagood commented Jan 30, 2020

Copy link
Copy Markdown
Member

You can reference $(MicrosoftNETCoreAppRuntimePackage).$(PackageRID), I don't think it's any more dangerous than referencing runtime.$(PackageRID).$(MicrosoftNETCoreAppPackage.ToLowerInvariant()).

@jkoritzinsky

Copy link
Copy Markdown
Member Author

Do we have $(PackageRID) during restore time? And what variable should we use for the version of the runtime pack?

@dagood

dagood commented Jan 30, 2020

Copy link
Copy Markdown
Member

Yeah, PackageRID is set statically. For the version, repos typically default to the win-x64 package for the dependency's sake, so MicrosoftNETCoreAppRuntimewinx64Version.

@jkoritzinsky

Copy link
Copy Markdown
Member Author

Ok. I'll change it to use that.

@jkoritzinsky

Copy link
Copy Markdown
Member Author

@dagood any idea what's causing the test timeouts? I can't imagine anything in this PR would cause that.

@dagood

dagood commented Jan 30, 2020

Copy link
Copy Markdown
Member

This project doesn't even have tests, let alone tests that run in Helix.

@dotnet/dnceng can you investigate why the tests are hanging?

@MattGal

MattGal commented Jan 30, 2020

Copy link
Copy Markdown
Member

Huh. The jobs that ended up cancelling didn't start for 1.5+ hours. I'll poke around a bit after standup.

@jkoritzinsky

Copy link
Copy Markdown
Member Author

In the meantime I'm going to try re-running this and see if we can get a clean build.

@MattGal

MattGal commented Jan 30, 2020

Copy link
Copy Markdown
Member

Basically I found nothing "wrong", rather the redhat.7.amd64.open queue took 3372 work items taking 2 days and 22 hours of work in that time frame (about 1:14) while the debian 9 one did 4017 work items totalling 3 days, 22 hours there. Astounding amounts of computing to get done in an hour, but I get the frustration.

What this reminds me of is that I need to create dedicated arcade queues for test execution. I will prioritize this work, my apologies for the inconvenience. (Will link back to this issue when I find it)

@jkoritzinsky

Copy link
Copy Markdown
Member Author

Thanks @MattGal for taking a look!

@dagood I got a fully green build on this PR. I don't have merge permissions in dotnet/arcade any more, so can you merge this in for me?

@MattGal
MattGal merged commit 117d426 into dotnet:master Jan 30, 2020
@MattGal

MattGal commented Jan 30, 2020

Copy link
Copy Markdown
Member

(Since Davis signed off already and it looks sane, I merged it)

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.

3 participants