Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Make ILAsm and ILDasm packages standalone. Clean up Microsoft.NET.Sdk.IL #25930

Merged

Conversation

jkoritzinsky
Copy link
Member

Now that ILAsm and ILDasm are standalone as of #25144, this PR updates the Microsoft.NETCore.ILAsm and Microsoft.NETCore.ILDasm packages to not depend on the CoreCLR package.

Additionally, this PR also cleans up the Microsoft.NET.Sdk.IL sdk since standalone ILAsm and ILDasm allows a lot of the MSBuild logic to be removed.

…ing. Don't pull down CoreCLR or ClrJit since ILAsm and ILDasm don't need them. Don't copy ILAsm or ILDasm to the output directory of the project when using the tools from packages.
@Nirmal4G
Copy link

I see that they refer the packages from NuGet. For an SDK, the tools shouldn't be self contained (to allow better portability, not depending on NuGet)?

@Nirmal4G
Copy link

Since the tools themselves are portable now, why not put them inside the SDK package, preferably a Tools folder besides the Sdk folder.

@jkoritzinsky
Copy link
Member Author

The tools are still architecture-specific native tools, so can't just package on one platform build. I don't think the MSBuild support for SDK resolution via NuGet supports runtime.json-based architecture specific packages, so we likely need to keep using the NuGet packages.

Since the <PackageReference>s are marked with IsImplicitlyDefined="true", they won't show up to the user and are encapsulated in the SDK.

@Nirmal4G
Copy link

so can't just package on one platform build

I see. Since, it's a private package, this does not matter. But you can also just do this:

Tools\<runtime-id>\<files>

Then again, the downside is, the native assets for every runtime-id would be there, contributing to the package size. You might not want other platform/arch assets on the platform/arch you're running from.

@Nirmal4G
Copy link

Anyway, since we have the compiler written in .NET, It would be nice to have IL tools and decompiler written in .NET too!

@jkoritzinsky
Copy link
Member Author

cc: @tannergooding @ericstj

Copy link
Member

@tannergooding tannergooding left a comment

Choose a reason for hiding this comment

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

LGTM.

Have we validated S.R.CS.Unsafe.ilproj builds correctly with this?

@jkoritzinsky
Copy link
Member Author

I did a local build with a test project and it restored the package correctly.

@jkoritzinsky jkoritzinsky merged commit 4e960d7 into dotnet:master Aug 8, 2019
@jkoritzinsky jkoritzinsky deleted the standalone-ilasm-ildasm-pkg branch August 8, 2019 23:32
@mletterle
Copy link

I noticed this fix has started showing up in 5.0 alpha builds on https://dotnetfeed.blob.core.windows.net/dotnet-coreclr but not on 3.1 preview builds, is this expected to ship in 3.1?

@jkoritzinsky
Copy link
Member Author

This is not expected to ship in 3.1. I don’t think the underlying change that enabled this is in 3.1.

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
….IL (dotnet/coreclr#25930)

* Remove reference to CoreCLR package from ILAsm and ILDasm packages.

* Directly reference ILAsm and ILDasm in their packages instead of copying. Don't pull down CoreCLR or ClrJit since ILAsm and ILDasm don't need them. Don't copy ILAsm or ILDasm to the output directory of the project when using the tools from packages.


Commit migrated from dotnet/coreclr@4e960d7
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