-
Notifications
You must be signed in to change notification settings - Fork 466
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Emit invocation types in same module as their associated proxy type #346
Emit invocation types in same module as their associated proxy type #346
Conversation
65a5af3
to
cd9921e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of minor notes / questions.
internal bool InStrongNamedModule | ||
{ | ||
get { return StrongNameUtil.IsAssemblySigned(TypeBuilder.Assembly); } | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
build.sh
Outdated
|
||
# 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that looks good to me, both GNU and BSD grep support multiple files.
build.sh
Outdated
if [ $NETCORE_FAILCOUNT -ne 0 ] | ||
then | ||
echo "NetCore Tests have failed, failing the build" | ||
exit 1 | ||
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(as above)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @stakx, check out my comments.
Castle.Core.sln
Outdated
@@ -31,6 +31,8 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Castle.Services.Logging.Ser | |||
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}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, my preference would be "strong" and "weak" named too.
build.sh
Outdated
|
||
# 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that looks good to me, both GNU and BSD grep support multiple files.
<RootNamespace>Castle</RootNamespace> | ||
<Version>0.0.0.0</Version> | ||
<AssemblyVersion>0.0.0.0</AssemblyVersion> | ||
<SignAssembly>False</SignAssembly> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll look into it.
I'm not sure this is going to work out so well.
What's your preference here? Go ahead with any of the above options, or keep a separate weak-named test project?
|
(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.
b322a8b
to
5aade79
Compare
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. |
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 applyingforceUnsigned
when generating invocation types won't have any undesired side effects.