Emit invocation types in same module as their associated proxy type by stakx · Pull Request #346 · castleproject/Core

stakx · 2018-04-22T23:54:31Z

My attempt at fixing #327, which is about DynamicProxy generating two invalid dynamic assemblies when proxying types from non-strong-named assemblies (because the proxy types end up in the weak-named assembly while the associated invocation types end up in the strong-named assembly).

This PR does two things:

Add an additional unit test project whose assembly does not get strong-named. Otherwise, we have no way of testing this issue (since the main unit test project gets signed).
Make sure that the dynamically generated invocation types end up in the same assembly as the generated proxy type.

~~Not to be merged just yet. I'd like to check some more that applying forceUnsigned when generating invocation types won't have any undesired side effects.~~

stakx

Just a couple of minor notes / questions.

stakx · 2018-04-23T00:33:08Z

src/Castle.Core/DynamicProxy/Generators/Emitters/ClassEmitter.cs

+		internal bool InStrongNamedModule
+		{
+			get { return StrongNameUtil.IsAssemblySigned(TypeBuilder.Assembly); }
+		}


This property gets checked for every invocation type being generated, so this might have a slight performance impact. I guess the result returned by StrongNameUtil.IsAssemblySigned could be cached here, in ClassEmitter. Seeing that StrongNameUtil already performs caching, all we'd really save at this point is the time it takes to enter and leave the lock inside StrongNameUtil. And caching here might actually introduce a lock of its own, so just going ahead and performing this call instead of caching the result is probably good enough.

stakx · 2018-04-23T00:36:06Z

build.sh


 # Unit test failure
-NETCORE_FAILCOUNT=$(grep -F "One or more child tests had errors" NetCoreClrTestResults.xml | wc -l)
+NETCORE_FAILCOUNT=$(grep -F "One or more child tests had errors" NetCoreClrTestResults.xml NetCoreClrNonSignedTestResults.xml | wc -l)


Can you confirm that this grep call syntax (mentioning several space-separated file names) is correct and will work correctly in the case of a failed unit test?

Yep, that looks good to me, both GNU and BSD grep support multiple files.

stakx · 2018-04-23T00:36:19Z

build.sh

 fi

-MONO_FAILCOUNT=$(grep -F "One or more child tests had errors" DesktopClrTestResults.xml | wc -l)
+MONO_FAILCOUNT=$(grep -F "One or more child tests had errors" DesktopClrTestResults.xml DesktopClrNonSignedTestResults.xml | wc -l)


jonorossi

Great work @stakx, check out my comments.

jonorossi · 2018-04-23T11:47:58Z

Castle.Core.sln

 EndProject
 Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Castle Services", "Castle Services", "{A598EE9B-41CE-4BE8-BF93-2C91F919F97E}"
 EndProject
+Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Castle.Core.Tests.NonSigned", "src\Castle.Core.Tests.NonSigned\Castle.Core.Tests.NonSigned.csproj", "{14D86762-CF9B-4560-80C9-10C16DBE246C}"


Could this and other places please use the term "non strong named" or "weak named", rather than "non signed" or "unsigned", since this is strong naming not code signing. I know some existing code is mixed and even Microsoft is inconsistent with naming between these two concepts.

Sure. I've actually been a little undecided about the correct term to use. I'll change the name to WeakNamed (mainly because it's shorter than NonStrongNamed).

Great, my preference would be "strong" and "weak" named too.

jonorossi · 2018-04-23T11:50:21Z

build.sh


 # Unit test failure
-NETCORE_FAILCOUNT=$(grep -F "One or more child tests had errors" NetCoreClrTestResults.xml | wc -l)
+NETCORE_FAILCOUNT=$(grep -F "One or more child tests had errors" NetCoreClrTestResults.xml NetCoreClrNonSignedTestResults.xml | wc -l)


Yep, that looks good to me, both GNU and BSD grep support multiple files.

jonorossi · 2018-04-23T12:25:09Z

src/Castle.Core.Tests.NonSigned/Castle.Core.Tests.NonSigned.csproj

+		<RootNamespace>Castle</RootNamespace>
+		<Version>0.0.0.0</Version>
+		<AssemblyVersion>0.0.0.0</AssemblyVersion>
+		<SignAssembly>False</SignAssembly>


It is going to really suck to have to maintain a nearly exact copy of the unit test project. Do you think we could get build the same project twice, with the second build passing a parameter to not strong name?

I'll look into it.

stakx · 2018-04-23T17:11:58Z

Do you think we could get build the same project twice, with the second build passing a parameter to not strong name?

I'm not sure this is going to work out so well.

Castle.Core's internals are made visible to Castle.Core.Tests. Since the former is strong-named, the latter needs to be strong-named as well. ([assembly: InternalsVisibleTo("Castle.Core.Tests")] in Castle.Core won't work if Castle.Core is strong-named but Castle.Core.Tests isn't.) If we want a build property that determines strong-naming vs. not strong-naming, it will have to be applied to both projects, not just to the test project.
Alternatively, we can surround all tests that access Castle.Core's internals (and the [InternalsVisibleTo] attribute) with a #if STRONG_NAMED guard whose presence depends on the build property. However, sprinkling more conditional compilation blocks across the test project isn't exactly neat, though.

What's your preference here? Go ahead with any of the above options, or keep a separate weak-named test project?

If we keep a separate weak-named test project, we could probably factor out some of the identical bits of both test projects into a MSBuild .props file.

stakx · 2018-04-24T08:52:43Z

(I forgot a fourth alternative: leaving this change untested, and therefore not having a weak-named test project at all.)

Prior to this commit, DynamicProxy, when asked to proxy a type from a weak-named assembly, would generate the proxy type in the weak-named dynamic assembly, and the associated invocation types in the strong- named dynamic assembly. This results in PEVerify rightfully claiming that both of them are invalid. Since the assembly of main test project (Castle.Core.Tests) gets signed, it's difficult to write tests regarding this issue. Therefore let's add a second test project that stays non-strong-named.

jonorossi · 2018-04-24T14:32:45Z

What's your preference here? Go ahead with any of the above options, or keep a separate weak-named test project?

Oops, I thought you were running all unit tests in both test assemblies, but the new weak named one just runs those new ones you added. Let's keep it with 2 projects, the weak named one will likely only need to be updated when new tests are added to it which won't be often.

stakx force-pushed the invocationtypes-module-affinity branch from 65a5af3 to cd9921e Compare April 23, 2018 00:00

stakx commented Apr 23, 2018

View reviewed changes

jonorossi requested changes Apr 23, 2018

View reviewed changes

stakx added 4 commits April 24, 2018 16:04

Emit invocation types in same module as proxy type

fdbee4e

Update the changelog

95944a2

Replace NonSigned with WeakNamed

5aade79

stakx force-pushed the invocationtypes-module-affinity branch from b322a8b to 5aade79 Compare April 24, 2018 14:04

jonorossi approved these changes Apr 24, 2018

View reviewed changes

jonorossi merged commit 9538390 into castleproject:master Apr 24, 2018

jonorossi added this to the vNext milestone Apr 24, 2018

jonorossi mentioned this pull request Apr 24, 2018

ModuleScope.SaveAssembly produces incorrect / incomplete assemblies when proxying types from a weak-named assembly #327

Closed

stakx deleted the invocationtypes-module-affinity branch April 24, 2018 15:21

stakx mentioned this pull request Apr 30, 2018

Add additional test run protocols to .gitignore #352

Merged

stakx mentioned this pull request Jan 30, 2021

DynamicProxy generates two modules when proceeding from a class proxy's protected method to the target #569

Closed

Conversation

stakx commented Apr 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stakx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonorossi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stakx commented Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stakx commented Apr 24, 2018

Uh oh!

jonorossi commented Apr 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

stakx commented Apr 22, 2018 •

edited

Loading

stakx commented Apr 23, 2018 •

edited

Loading