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

Remove old transport packages from CoreCLR and CoreFX. #5523

Merged

Conversation

jkoritzinsky
Copy link
Member

Now that dotnet/arcade#4708 is merged, the last user of a number of our old transport packages is gone. So, we should be able to remove most of our old transport packages:

  • Microsoft.NETCore.Jit
  • Microsoft.NETCore.Runtime.CoreCLR
  • Microsoft.TargetingPack.Private.CoreCLR
  • Microsoft.Private.CoreFx.NETCoreApp

We need to keep the IL packages since they're used via the IL SDK. We need to keep the Microsoft.NETCore.TestHost package since the diagnostic-tests repository still consumes it.

This PR also removes some now-dead code from the coreclr packaging infrastructure.

Fixes #126

@jkoritzinsky jkoritzinsky added this to the 5.0 milestone Jan 30, 2020
@jkoritzinsky jkoritzinsky requested review from hoyosjs and a team January 30, 2020 23:31
Copy link
Member

@trylek trylek left a comment

Choose a reason for hiding this comment

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

Looks great to me, thank you!

Copy link
Member

@hoyosjs hoyosjs left a comment

Choose a reason for hiding this comment

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

LGTM, although I just have some questions:

  • What's going to happen with libtracepointproviders.so on unsupported architectures? Before we really excluded them from packaging. Tizen had issues with it for example.
  • Now that the long-name generation of the DAC happens in CMake, will this still exist in the runtime packages?
  • What's the packaging for the cross-arch DAC (and it's long name variant)? Right now they live under artifacts\bin\coreclr\Windows_NT.arm64.Debug\x64\sharedFramework

@jkoritzinsky
Copy link
Member Author

  • What's going to happen with libtracepointproviders.so on unsupported architectures? Before we really excluded them from packaging. Tizen had issues with it for example.
  • Now that the long-name generation of the DAC happens in CMake, will this still exist in the runtime packages?
  • What's the packaging for the cross-arch DAC (and it's long name variant)? Right now they live under artifacts\bin\coreclr\Windows_NT.arm64.Debug\x64\sharedFramework

As is the case with all native CoreCLR files, if it gets installed in CMake to the sharedFramework folder, it will be packaged. If it is not installed to the sharedFramework folder, it won't be packaged.

This change makes no changes to the packages we ship.

@ghost
Copy link

ghost commented Feb 5, 2020

Hello @jkoritzinsky!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Copy link
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

We should clean up

<BinPlaceConfiguration Condition="'$(IsNETCoreApp)' == 'true' and '$(BuildingNETCoreAppVertical)' == 'true'" Include="$(NetCoreAppCurrent)-$(_bc_OSGroup)">
. Can we stop using the props file generation and instead just use simple file copies?

<IncludeSymbolsInPackage Condition="'$(IncludeSymbolsInPackage)' == ''">true</IncludeSymbolsInPackage>
</PropertyGroup>

<ItemGroup>
Copy link
Member

Choose a reason for hiding this comment

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

@dagood do we have closure validation and type dup checking in final packages?

Copy link
Member

Choose a reason for hiding this comment

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

We should have it, but I'm not able to find evidence of it in an official build binlog... targets appear to noop and I don't see why at the moment. Filed #31805.

@jkoritzinsky
Copy link
Member Author

I don't know enough about the binplacing system in the libraries build to feel comfortable switching it to something different in this PR. I'd prefer to leave that to a follow up PR.

@ericstj
Copy link
Member

ericstj commented Feb 5, 2020

I don't know enough about the binplacing system in the libraries build to feel comfortable switching it to something different in this PR. I'd prefer to leave that to a follow up PR.

That’s fair. We can follow up to change it.

@jkoritzinsky
Copy link
Member Author

Test failure is #2176
Merging.

@jkoritzinsky jkoritzinsky merged commit adc4e4c into dotnet:master Feb 5, 2020
@jkoritzinsky jkoritzinsky deleted the remove-intermediate-packages branch February 5, 2020 22:13
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 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.

Remove private CoreFx and CoreClr transport packages
8 participants