Repository navigation
Make ILCompiler.Reflection.ReadyToRun an Official NuGet Package #41379
Description
Background
ILSpy uses a private NuGet created by @cshung to implement the ReadyToRun decompilation functionality. The private NuGet is mostly copies of \runtime\src\coreclr\src\tools\aot\ILCompiler.Reflection.ReadyToRun plus a few common files that are infrequently updated manually.
Private NuGet inclusion see https://github.com/icsharpcode/ILSpy/blob/master/ILSpy.ReadyToRun/ILSpy.ReadyToRun.csproj#L78
Proposed Solution
Although using this private NuGet gets the job done, taking verbatim copies from "some other place" is a practice we'd like to stop rather sooner than later. The preferred fix is to have an official NuGet package for ILCompiler.Reflection.ReadyToRun - much like we are using System.Reflection.Metadata today.
Activity
MichalStrehovsky commentedon Aug 26, 2020
When you say "official", do you mean: "is tested, has guarantees around breaking changes to the API surface, and gets published by Microsoft to nuget.org"? Or is this more a request for "a package gets pushed into an Azure NuGet feed with the rest of the artifacts produced by daily builds of dotnet/runtime".
Cc @mangod9
christophwille commentedon Aug 26, 2020
We'd be fine with the latter - built & provided by dotnet/runtime team (as long as the retention on that feed is for a long-ish time). We are aware that the potential number of consumers for this package might be too small for a full-on nuget.org release with all the necessary quality guarantees.
sos-dll commentedon Aug 27, 2020
tldr: For convenience reasons. It would be appreciated. 👍
Bear with us (consumers)
Christoph, you beat me to it. (I was looking for this library on nuget yesterday and didn't find it, and now here's a request..)
It would be nice to avoid using some private feed (or alternatively stop wasting time pulling-compiling and setting up a local feed).
Can't speak to whether it is "stable and passes QA" or not, it just works so far consequently I would also agree to just do alpha/nightly/preview/daily-type of release.
mangod9 commentedon Aug 27, 2020
@cshung will follow up on the requirements here and we will evaluate the best approach to get it officially published.
cshung commentedon Aug 28, 2020
I am writing this comment to propose and confirm the requirements for publishing.
Just to make sure everybody is on the same page.
ILCompiler.Reflection.ReadyToRunis a library I extracted fromR2RDump- a tool for doing a text dump of a ready to run image. The tool is routinely used by the @dotnet/crossgen-contrib team to develop the ready to run compilers.Retention and Frequency
Retention is probably the most important concern. For projects to reliably depend on a library, it is important for them to make sure the library exists. This is probably more important than the release frequency. Looking at history, the library is not changed often enough to warrant a daily release. So I suggest a release that lasts forever but on-demand, similar to what we did with
Microsoft.Diagnostics.Tracing.TraceEvent.Testing
We do not have any automated testing of the library. That being said,
R2RDumpis routinely used by the team. The tool exercises most of the code in the library and therefore we get good coverage there. So I suggest testing through routineR2RDumpusage is sufficient.Breaking changes
The ready to run compiler is still an active project. As such, there will be changes. Making sure the library is backward compatible without breaking changes is more a hindrance than a help to the project. Thanks to NuGet, as long as the referencing projects do not upgrade, breaking changes should not be a problem. The
0.y.zversion should be used to reflect that fact. History taught us that adapting to some simple breaking changes is relatively straightforward. So I suggest we should allow breaking changes.Publishing Feed
I am not sure if we need a certain level of guarantees in order to be published through
Nuget.org. But unless there is a reason why we cannot be released throughNuGet.org, I suggest we should release it throughNuget.org, this is going to be more discoverable.Feedback
@christophwille, @sos-dll
Please let us know if my suggestion is good enough for your purposes, feel free to raise any concern.
FYI @tmds.
Microsoft internal folks, if my suggestions are impractical for any reason, feel free to let me know.
davidwrighton commentedon Aug 28, 2020
As for retention and frequency, we should have our standard nuget publishing behavior where we publish to a development feed on every official build, and we will publish on occasion to actual nuget. At some point we should probably consider transforming PerfView to use this as the means for generating pdb files, so we'll want a means to do that.
We need to make sure we sign the binaries.
Testing, the ability of R2RDump to successfully dump framework dlls has been problematic, and there have been repeated problems. I would like to see a plan put together for testing which runs r2rdump on System.Private.Corelib, and one other large framework assembly. Possibly we could have a single test which spawns r2rdump and runs it on System.Private.CoreLib, some other framework dll, and if present the composite R2R image that exists when doing a composite test pass.
jkotas commentedon Aug 28, 2020
Another topic to cover is Security: This is parser for potentially malicious input. Are there things to do for that?
christophwille commentedon Aug 29, 2020
Good enough for our purposes.
mangod9 commentedon Jul 10, 2021
@cshung this can be closed now I assume?
cshung commentedon Jul 10, 2021
Yes, we still need to do the security review, but that can be tracked by #42147