Skip to content
This repository was archived by the owner on Jun 21, 2023. It is now read-only.

Make tests pass when PR submitted by an external contributor #1690

Merged
merged 1 commit into from
May 18, 2018
Merged

Make tests pass when PR submitted by an external contributor #1690

merged 1 commit into from
May 18, 2018

Conversation

jcansdale
Copy link
Collaborator

@jcansdale jcansdale commented May 18, 2018

What this PR does

There is a catch-22 when using the InternalsVisibleTo attributes:

  1. Signed assemblies can only reference other assemblies assemblies.
  2. Signed assemblies can only have signed "friend" assemblies.

This means that when the the GitHub.Exports assembly is unsigned, it can't be referenced by a signed test assembly. Unfortunately, if the test assembly is unsigned, it can't be used as a "friend" assembly when GitHub.Exports is signed. This means that either everything must be signed or everything must be unsigned.

  • Don't strong name test assemblies
  • Don't use StrongNameSigner
  • Use a nested class called Internal instead of InternalsVisibleTo attribute

Testing

I created this using the new fork functionality and am submitting it as an external contributor. If everything goes to plan, the tests will pass. ✅

This is currently impacting #1655 and #1690

Fixes #1669

Copy link
Contributor

@grokys grokys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might it be better to just sign everything with a (different) public key when the user doesn't have access to our key?

@jcansdale
Copy link
Collaborator Author

jcansdale commented May 18, 2018

Might it be better to just sign everything with a (different) public key when the user doesn't have access to our key?

I'd be inclined to do something like that as well. Alternatively we could have one key for Debug builds and another key for Release builds. The Debug key could be public and the Release key could be private. Doing this the Release version would be less likely to interfere with the Debug version when installed in the experimental instance (the assemblies would have different identities). Contributors would be able to do a Debug build using the exact same configuration as us. 🎉

Re: the test assemblies, I'd prefer to avoid strong naming them if possible because this limits dependencies to be strong named. This project was using StrongNameSigner, but that lead to some very unhelpful errors when the tests stopped working. 😕

@grokys
Copy link
Contributor

grokys commented May 18, 2018

Not strong naming the test assemblies definitely makes sense. As for signing Debug and Release builds differently, we should maybe talk about that with @shana.

@jcansdale
Copy link
Collaborator Author

As for signing Debug and Release builds differently, we should maybe talk about that with @shana.

This one is worth taking a little time to think about. It's a different idea to what has been floated before. We wouldn't be using "open source" signing, where the assemblies would end with the same identity as the released version. We'd be giving Debug and Release versions different identities.

Assuming people generally install the Release/gallery version on their local instance and F5 the experimental instance of VS using the Debug configuration, it should have the happy consequence of avoiding the installation issues people have been having. 😄

- Don't sign test assemblies
- Don't use InternalsVisibleTo
Copy link
Contributor

@grokys grokys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM I think!

@jcansdale jcansdale merged commit d308872 into github:master May 18, 2018
@jcansdale jcansdale deleted the fixes/1669-do-not-strong-name branch May 18, 2018 15:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants