Cecil binaries need to be internalized

gluck / il-repack

Open-source alternative to ILMerge

Apache License 2.0

1.16k stars 214 forks source link

Cecil binaries need to be internalized #358

Closed KirillOsenkov closed 5 months ago

KirillOsenkov commented 5 months ago

See https://github.com/gluck/il-repack/issues/357#issuecomment-2035275096

KirillOsenkov commented 5 months ago

This is because Cecil API bleeds into ILRepack public API, so for consistency the transitive closure of Cecil exposed publicly needs to be public:

Processing public types tree Public API: Forcing type ILRepack::Mono.Cecil.AssemblyAttributes to public because of: ILRepacking.ILRepack -> OtherAssemblies -> Generic type argument AssemblyDefinition -> Mono.Cecil.AssemblyDefinition -> Name -> Mono.Cecil.AssemblyNameDefinition -> base type -> Mono.Cecil.AssemblyNameReference -> Attributes -> Mono.Cecil.AssemblyAttributes

I'll see if we can make some ILRepack API internal as to avoid exposing Cecil API.

KirillOsenkov commented 5 months ago

Versions < 2.0.21 used to produce invalid assemblies where public API mentioned internal types. 2.0.21 and later fixes this by automatically "publicizing" the transitive closure of the main assembly's public API

This was done here: https://github.com/gluck/il-repack/commit/fed76eead2fbea469234285c3fe97ee6ca2b6614

KirillOsenkov commented 5 months ago

@mattleibow I think we're in a bit of a pickle here. ILRepack liberally smeared Cecil types all over its public API. If we force Cecil types to internal it will be an invalid assembly (not sure how does 2.0.18 even work?)

If we make all parts of ILRepack API internal that mention Cecil, we might break consumers who expect that API.

Can you point me to your usage(s) of the ILRepack API? I'm guessing you're using https://www.nuget.org/packages/ilrepack.lib?

KirillOsenkov commented 5 months ago

Ideally I think ILRepack shouldn't have exposed Cecil as it's an implementation detail. If suppose in the future we want to switch from Cecil to System.Reflection.Metadata, we should be able to do it without impacting consumers.

I'd be curious if anyone depends on the aspects of ILRepack public API that has Cecil types.

KirillOsenkov commented 5 months ago

I decided to risk it and made 10 members internal and that resulted in Cecil being properly internalized. If this breaks someone else, my apologies, we'll look into that next.

KirillOsenkov commented 5 months ago

Published 2.0.30, please try it out: https://www.nuget.org/packages/ILRepack/2.0.30 https://www.nuget.org/packages/ILRepack.Lib/2.0.30

mattleibow commented 5 months ago

Amazing. I'll try it out ASAP.