Strong naming MedallionShell assembly

[madelson]

Strong naming MedallionShell assembly

[madelson]

Hi @ldennington thanks for your interest in the library!

I'll admit that I've never worked with strong naming before (other than tweaking binding redirects) but after reading over some of the MSFT docs on this (e. g. https://github.com/dotnet/runtime/blob/master/docs/project/strong-name-signing.md) it seems like something I'd be happy to have the library support. I'd be inclined to follow MSFT's recommendation for open source and have the private key checked into the library. I've forked other projects before where the build fails due to the key being missing and that is always frustrating.

Given my lack of knowledge, if you are willing to take a first crack at this and submit a PR that would be great.

The one thing I can think of that might be slightly tricky is that the main MedallionShell assembly embeds the ProcessSignaler assembly inside of it. I'm curious whether you think both should be strong-named or just MedallionShell. If both, then we'll need to make sure that everything happens in the right order during the build (hopefully this will just work).

[ldennington]

Ah yes. Unfortunately, signing only the MedallionShell and MedallionShell.ProcessSignaler assemblies was the first option we tried, as that was our preference too. However, since the MedallionShell.Tests assembly references MedallionShell and needs to access its internal methods, we had to strongly sign it (the build failed before we did so). And, since MedallionShell.Tests also references the SampleCommand assembly, that assembly had to be strongly signed as well before the build would pass.

[kyle-rader]

Sorry I also can't seem to find any comments on the PR itself.

As @ldennington mentioned the Tests have to be signed as-is because MedallionShell is using the InternalsVisisbleTo attribute to specify the tests project has access to internal methods within the library (likely so they can be test). A strong name signed assembly can only do grant this to other strong name signed assemblies.
How to: Create signed friend assemblies for reference.

[ldennington]

Hi @madelson -

I'm so sorry for not highlighting this issue; I actually didn't realize strong naming the assembly would break consumers (if it's not obvious already I've been learning as I go through this process too 😀).

I think your solution is more than reasonable. I really appreciate your willingness to maintain a separate branch with a strong name variant until you're ready for 2.0, as that will definitely provide what I need.

Please let me know if there's anything I can do to be of assistance with the initial strong name release.

[ldennington]

Thanks for the super-speedy followup @madelson!

I've submitted a PR with these changes here. Let me know if you see any issues, although I think it's pretty straightforward.

Also, when you have a moment, would you mind sharing your workflow for packaging these binaries? I'm working on publishing a nuget package locally to validate my changes and want to make sure I'm following the correct series of steps!

[madelson]

Awesome thanks! I've added some comments/questions. The main simplification I'd hope to see would be avoiding signing the tests and samplecommand assemblies unless we have to for some reason.

Also, when you have a moment, would you mind sharing your workflow for packaging these binaries? I'm working on publishing a nuget package locally to validate my changes and want to make sure I'm following the correct series of steps!

All I do is build in release mode which should generate the package on build. Does that work for you?

[ldennington]

Also I'm not seeing any comments unfortunately - is it possible your review was not published? Or am I just overlooking them?

[madelson]

Sorry looks like it is now submitted.

Main remaining comment/question is:

Is it necessary to use distinct snk files for each assembly? I feel like with MSFT assemblies I always see the same public key used (in binding redirects), which is kind of nice for knowing you've copied it correctly.

EDIT: one more question concerning the release.

In some places, where NuGet assemblies are strong-named I know that package authors will keep the assembly version at Major.0.0.0 rather than having it track with the NuGet package version. The idea is that this avoids the need for binding redirects unless you have references to different major versions. The downside is that assembly.version is a bit misleading. See https://codingforsmarties.wordpress.com/2016/01/21/how-to-version-assemblies-destined-for-nuget/. Have you come across this practice? Any thoughts as to whether it is a good idea in this case?

[ldennington]

I agree, so I updated to consolidate on a single MedallionShell.snk file. I don't have a ton of experience with NuGet vs Assembly naming - @kyle-rader can you weigh in on that?

[madelson]

@ldennington thanks! I'll try to get this merged and published over the weekend. I'm definitely interested in hearing thoughts on the versioning question but that doesn't need to block this from going out.

[ldennington]

Thanks so much @madelson - getting a release this weekend would be super helpful! @kyle-rader gentle ping in case you have thoughts on the above.

[madelson]

Hi @ldennington .

Minor bump in the road. I was reading on https://docs.microsoft.com/en-us/dotnet/standard/library-guidance/breaking-changes that

Renaming an assembly with AssemblyName will change the assembly's identity, as will adding, removing, or changing the assembly's strong naming key. A change of an assembly's identity will break all compiled code that uses it.

If I understand correctly, in order to switch this, we'd have to jump to a 2.0 release because we'll break backwards compat with all existing versions. I'm reluctant to do that at this moment. My philosophy is that I strive to keep breaking changes to an absolute minimum because they can be so disruptive to consumers. Therefore, my plan has been for the eventual 2.0 release to take the opportunity of a 2.0 release to revisit the APIs and functionality more broadly, likely bundling several breaking changes together and then hopefully waiting for a very long time before jumping to 3.0. Hopefully this makes sense.

However, that doesn't mean we have to hold off on a release here. My thought is that, for the duration of 1.x, I will maintain a strong-name branch in the repo from which I will publish a MedallionShell.StrongName variant of the package. This is something I see other packages doing (see https://www.nuget.org/packages?q=strongname), perhaps for similar reasons. This branch will become the basis for that branch. For any further releases of 1.x, I'll also merge them in to the strong name branch and release there. One I'm ready for 2.0, I'll merge the StrongName branch into master and 2.0+ of MedallionShell will have strong names.

Hopefully this will still allow you to get what you want out of this?

[madelson]

@ldennington new package is out! See https://www.nuget.org/packages/MedallionShell.StrongName/.

Thanks again (also @kyle-rader) for contributing :-)