-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add strong name signing #2575
Add strong name signing #2575
Conversation
Hey @AndreyTretyak Thank you for the work here ❤️. I feel like a lot of folks have been waiting for this. I think I'd like to push this after our next release just so that it is an encapsulated change - I'll most likely release it as a breaking change as well, given the type of change it is. |
Thank you, Nick. I'm looking forward to this. Are there any plans for when the next and following releases might happen? |
Hey @AndreyTretyak, We'll try to drop what is in |
Hey @AndreyTretyak, We just dropped our last release before we get this merged in on Monday. Just a quick question - was there any reason you didn't end up signing Octokit.Reactive? If not, let me know if you'd be able to get that one done or if you'd like me to do it in your branch. I'd like to get both done, so we only have to release one breaking change. Let me know what you think. Thanks again for getting this hammered out! |
Just checked and |
Perfect. Thanks for validating that... we should be g2g on Monday then. 💥 |
@AndreyTretyak, |
It's a follow-up after my comment here #1604 (comment) that intended to resolve #1604, #2382, and similar issues. I would be grateful if you would consider this change since it would simplify my team's life significantly.
Main justification points:
Octokit
before version1.0
, and currently we are at version3.0
, so I hope we can do this long overdue change.Octokit
packages likeOctokit.GraphQL
are signed for 5 years already (PR link) and it would be nice to make them consistent.StrongNamer
as a workaround is a constant source of build and debug problems, since it's essentially decompilingOctokit.dll
and compiling it again with the strong name during each build. So customers running not the dll that you've shipped.Implementations details:
Signing logic is done in the same way as in
octokit.graphql.net
using the same key for consistency and also without versioning changes.Since signing is enabled in the
Directory.Build.props
it would be applied by default to every new project created in the future in this repository without additional effort.If for some reason there is a need of disabling strong naming for the specific project this could be done by setting
<SignAssembly>false</SignAssembly>
in the project. As currently done forBuild.csproj
andOctokit.Tests.Integration.csproj
that reference unsigned dlls.Another slight change is in applying
InternalsVisibleTo
attribute for signed dlls. It should be done in the project file directly and with$(StrongNameSuffix)
after dll name (to include the public key without duplicating this horrible string).Side note: One of the options discussed in the epic thread here was to change dll version only on major version changes of NuGet package (Newtonsoft.Json model) and if needed this could be easily achieved by using declaring
AssemblyFileVersion
MSBuild property with the required dll version. Although I'm not sure how useful it is now when SDK-based projects are able to automatically generate binding redirects for executables during the build.