Skip to content

Added Test.snk and related fixes from the buildtools repo - #6203

Closed
harishsk wants to merge 1 commit into
dotnet:mainfrom
harishsk:testsnk
Closed

Added Test.snk and related fixes from the buildtools repo#6203
harishsk wants to merge 1 commit into
dotnet:mainfrom
harishsk:testsnk

Conversation

@harishsk

Copy link
Copy Markdown

ML.NET still relies on buildtools and I am in the process of migrating it to Arcade. ML.NET uses different keys for strong name signing the shipping binaries and the test binaries. Arcade has support Open.snk for strong name signing the shipping binaries but not for Test.snk for signing the test binaries.

This PR adds Test.snk from buildtools and adds related support in the Arcade props and targets files.

@MattGal MattGal left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Needed for teams porting their repos off BuildTools

@tmat

tmat commented Sep 23, 2020

Copy link
Copy Markdown
Member

Why do test assemblies need to be strong-name signed?

@MattGal

MattGal commented Sep 23, 2020

Copy link
Copy Markdown
Member

Why do test assemblies need to be strong-name signed?

So they can directly call privates: https://docs.microsoft.com/en-us/dotnet/api/system.runtime.compilerservices.internalsvisibletoattribute?view=netcore-3.1

(note this is test-signing only)

@harishsk

Copy link
Copy Markdown
Author

ML.NET allows some of the test assemblies access to the internals of the shipping assemblies for testing purposes. The internals that are shared are different between the shipping assemblies and test assemblies in some cases. So the test assemblies are signed with a different key than the shipping assemblies.

@tmat

tmat commented Sep 24, 2020

Copy link
Copy Markdown
Member

I see, but why in that case do you need a different key? Using the same key simplifies things.

@harishsk

Copy link
Copy Markdown
Author

Two keys are used to distinguish access to internals by other shipping binaries from access by test binaries.

@tmat

tmat commented Sep 24, 2020

Copy link
Copy Markdown
Member

What benefit does this distinction have? In Roslyn repo we use the same key for all binaries.

@harishsk

Copy link
Copy Markdown
Author

@eerhardt Can you please help with the question above? This was done prior to my time and I am not sure of the rationale for this. Is it okay to change ML.NET to be strong name signed with the same key for all binaries?

@eerhardt

Copy link
Copy Markdown
Member

why in that case do you need a different key?

That's just the way it was done in dotnet/corefx at the time that dotnet/machinelearning was set up. We basically snap-shotted the build infrastructure from corefx and cloned it to dotnet/machinelearning and made it work for ML.NET. Then we left it alone, until now.

In dotnet/runtime the tests use the Open.snk key for strong naming. I think we could convert dotnet/machinelearning tests to use that instead. We will need to update a few InternalsVisibleTo with the new public key. But it should be relatively straight forward. That way we wouldn't need to add yet another key to dotnet/arcade.

@harishsk

Copy link
Copy Markdown
Author

@eerhardt Thanks for the explanation.

It sounds like there is no impact if we change ML.NET to all be signed with the same key for strong naming. I will go ahead and do that. And once I have verified that it works, I will abandon this PR.

Thank you everybody.

Base automatically changed from master to main March 8, 2021 23:09
@jonfortescue

Copy link
Copy Markdown
Contributor

@harishsk Looks like there hasn't been movement on this in a while. Is this still something that ML.NET wants?

@harishsk

Copy link
Copy Markdown
Author

I don't believe this PR is necessary anymore. But I will let @michaelgsharp make the final call.

@michaelgsharp

Copy link
Copy Markdown
Contributor

Its not. We changed the certificates as suggested by @eerhardt so we are good.

@ChadNedzlek

Copy link
Copy Markdown
Contributor

This seems to be stalled. Is this still relevant, or should we close the PR?

@ghost

ghost commented Oct 4, 2021

Copy link
Copy Markdown

I'm just closing it based off this:

Its not. We changed the certificates as suggested by @eerhardt so we are good.

@ghost ghost closed this Oct 4, 2021
@eerhardt

eerhardt commented Oct 4, 2021

Copy link
Copy Markdown
Member

Thanks for closing, @riarenas. I can confirm this is no longer necessary.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants