search

ravibpatel / ILRepack.Lib.MSBuild.Task

MSBuild task for ILRepack which is an open-source alternative to ILMerge.
Other
107 stars 30 forks source link

Internalize not working with 2.0.22 #44

Closed antineutrino closed 7 months ago

antineutrino commented 8 months ago

The following image generates this Assembly image

It worked with 2.0.18.

KirillOsenkov commented 8 months ago

There's been an intentional change of behavior where if a class is reachable by the public API of the main assembly, it will no longer be internalized, to prevent inconsistencies in access modifiers. It seems that InvalidDataException may be used by some other public API?

Also, internalize doesn't internalize types in the main assembly. This is also by design.

KirillOsenkov commented 8 months ago

I see some other types in the Persistence namespace such as ConnectionInfo, IDbTracer, etc. seem to be internal.

antineutrino commented 8 months ago

The public types are not used anywhere in the solution where the repacking is happening. Is there some special behavior when it comes to exception types? image

KirillOsenkov commented 8 months ago

Hard to say without having access to your assemblies to investigate.

The source code is here if you want to debug: https://github.com/gluck/il-repack/blob/master/ILRepack/Steps/PublicTypesFixStep.cs

antineutrino commented 8 months ago

ILRepackBug.zip

The problem can be reproduced with this solution. Exception types seem to have some special behavior.

antineutrino commented 8 months ago

If I understand the code correctly, the issue seems to be this code: image https://github.com/gluck/il-repack/commit/0ce59b14493dae9772c6c677c043baa8775748dd#diff-5c1acbbbdc2379afb7f358e73fd668909104afaa4fd792bee05b481000647abe

Serializable types (like exceptions) are always not internalized.

A way to get the old behavior would be nice.

KirillOsenkov commented 8 months ago

How's this?

https://github.com/gluck/il-repack/commit/175f15b72225aa3b15d80e9a7d4360be49059222

antineutrino commented 8 months ago

Hi, I think this works in principle.

But there seems to be an issue when parsing the command line. image

Without this line the new cli argument is in the input assemblies list image This leads to an error later on.

KirillOsenkov commented 8 months ago

interesting, thanks for testing it! I'll look at the fix tomorrow, it's actually surprising behavior that I might want to fix

KirillOsenkov commented 8 months ago

My apologies once again! I wasn't very experienced with this command line API. Found the problem here: https://github.com/gluck/il-repack/commit/b8986b8c5a768de2769c3e767cc7e171355613d8

How's this?

antineutrino commented 8 months ago

Looks good to me.

antineutrino commented 8 months ago

Any plans to release a new BuildTask version?

KirillOsenkov commented 8 months ago

I just published https://www.nuget.org/packages/ILRepack/2.0.26

and sent this PR to update this repo: https://github.com/ravibpatel/ILRepack.Lib.MSBuild.Task/pull/48