Skip to content
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 option to disable all strong name signing during build of NuGet.Client repository #5744

Merged
merged 5 commits into from
Apr 11, 2024

Conversation

jeffkl
Copy link
Contributor

@jeffkl jeffkl commented Apr 8, 2024

Bug

Fixes: https://github.com/NuGet/Client.Engineering/issues/2790

Regression? Last working version:

Description

This change makes it possible to build our repository without any strong name signing enabled via SkipSigning=true. This will allow us to have a build pipeline that doesn't require disabling strong name validation and external contributors can build and run tests easier.

  • Moved all of the InternalsVisibleTo logic from AssemblyInfo.cs to the projects so we can adjust the public key token depending on if strong name signing is enabled
  • Added a new strong name key to sign tests with so that all InternalsVisibleTo are using the same public key.
  • Updated logic used during ILMerge to use the same key as the containing project.

This doesn't currently change any behavior other than tests using a different key. You can just now build with /property:SkipSigning=true at the command-line to build without any signing.

PR Checklist

  • PR has a meaningful title

  • PR has a linked issue.

  • Described changes

  • Tests

    • Automated tests added
    • OR
    • Test exception
    • OR
    • N/A
  • Documentation

    • Documentation PR or issue filled
    • OR
    • N/A

@jeffkl jeffkl self-assigned this Apr 8, 2024
@jeffkl jeffkl requested a review from a team as a code owner April 8, 2024 23:41
@@ -90,11 +90,6 @@
<PackageIcon>icon.png</PackageIcon>
</PropertyGroup>

<PropertyGroup>
<NUGET_PFX_PATH>$(MSBuildThisFileDirectory)..\keys\NuGetKey.snk</NUGET_PFX_PATH>
<MS_PFX_PATH>$(MSBuildThisFileDirectory)..\keys\35MSSharedLib1024.snk</MS_PFX_PATH>
Copy link
Member

Choose a reason for hiding this comment

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

build.ps1 override these when -SkipDelaySign was passed as an argument. It should be updated.

It possibly also makes this PR redundant, since there's already a different way to achieve the same outcome?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing that out. I've updated the script to set SkipSigning=true to just disable all strong name signing. This pull request will make it easier for our CI on all platforms to disable signing. I'm thinking in the near future to instead use SignType values of None (default), Test, and Real. This would be more in line with MicroBuild. But from a developer's perspective, we just build and run tests. I need to find a way to make it so we don't have to do anything special to debug our VSIX (if possible) and have the default signing to None. Otherwise I'll default signing to "Test" which will delay sign things and require strong name signing validation exclusions. But for Linux and Mac CI, we'll just build and run tests as unsigned.

@zivkan
Copy link
Member

zivkan commented Apr 9, 2024

  • PR has a meaningful title

it does not at the moment.

@jeffkl jeffkl changed the title ***NO_CI*** WIP Add option to disable all strong name signing during build of NuGet.Client repository Apr 9, 2024
@jeffkl jeffkl marked this pull request as draft April 9, 2024 17:35
@jeffkl jeffkl marked this pull request as ready for review April 10, 2024 00:09
@jeffkl
Copy link
Contributor Author

jeffkl commented Apr 10, 2024

Official build is now passing: https://dev.azure.com/devdiv/DevDiv/_build/results?buildId=9389345

I also verified a few assemblies to make sure their public key token didn't change. We can merge this and if it causes any issues we just can just revert and try again.

@jeffkl jeffkl merged commit 1504231 into dev Apr 11, 2024
13 of 16 checks passed
@jeffkl jeffkl deleted the dev-jeffkl-skipsigning branch April 11, 2024 20:25
donnie-msft pushed a commit that referenced this pull request Apr 12, 2024
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.

3 participants