Skip to content

Conversation

@hymccord
Copy link
Contributor

@hymccord hymccord commented Jun 1, 2022

Closes #254

Enable strong-name signing for all assemblies. This fixes a regression introduced in the repo migration. v7 was strong-named signed with the key that existed in the root of the repo.

PR Checklist

  • Created a feature/dev branch in your fork (vs. submitting directly from a commit on main)
  • Based off latest main branch of toolkit
  • PR doesn't include merge commits (always rebase on top of our main, if needed)
  • Tested code with current supported SDKs
  • Contains NO breaking changes
  • Code follows all style conventions

Other information

N/A

Copy link
Member

@Sergio0694 Sergio0694 left a comment

Choose a reason for hiding this comment

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

Can you also double check with a package from our PR feed to ensure it solves the issue for you? 🙂

@Sergio0694 Sergio0694 added the CI/pipeline 🔬 Some changes or issues related to CI infrastructure label Jun 1, 2022
@hymccord
Copy link
Contributor Author

hymccord commented Jun 1, 2022

See my discussion inside the original issue. You cannot use IsCoreProject (which is exactly what I did at first 😅) because InternalsVisibleTo doesn't work without a strong name on the unit test project.

@Sergio0694
Copy link
Member

Ok but then I'm confused as to how can the projects in the WCT use [IVT] without signing the tests though 🤔
@azchohfi would you happen to know how that's setup there?

@hymccord
Copy link
Contributor Author

hymccord commented Jun 1, 2022

I can confirm I am seeing signed assemblies from the PR artifacts

image

@Sergio0694
Copy link
Member

Uh... Why is the company ".NET Foundation" though. It should be "Microsoft".
Anyway that's a separate point, but @michael-hawker we should clarify and align all those line items I think 🙂

@hymccord
Copy link
Contributor Author

hymccord commented Jun 1, 2022

Uh... Why is the company ".NET Foundation" though. It should be "Microsoft". Anyway that's a separate point, but @michael-hawker we should clarify and align all those line items I think 🙂

That would be handled here:

<Copyright>(c) .NET Foundation and Contributors. All rights reserved.</Copyright>

I can open another issue for that one

@Sergio0694
Copy link
Member

Oh, no yeah I know where that comes from in the .props, I meant that as "why did we set that up this way" 😄

@hymccord
Copy link
Contributor Author

hymccord commented Jun 1, 2022

Looking at the v7.1.2 tag of CommunityToolkit/WindowsCommunityToolkit, none of the netframework-supported projects ever used IVT. Only in this repo does that happen. And with the given .targets file in the original repo, assembly signing is disabled for any uap10 targeted builds. You can see that in the NuGet packages as well.

I'm not well versed in UWP and how strong-naming works there. As I said in the issue discussion, I checked both the roslyn and runtime (coreclr subfolder) repos and everything is signed there.

@azchohfi
Copy link
Contributor

azchohfi commented Jun 1, 2022

@azchohfi would you happen to know how that's setup there?

I honestly don't remember... I don't remember that the tests had to be signed. If the tests were signed, then yes, everything they reference should be signed, but not the other way around, right?

@hymccord
Copy link
Contributor Author

hymccord commented Jun 2, 2022

but not the other way around, right?

Not always. IVT attribute requires that the friend assembly be signed as well if the one using the attribute is signed.

In one case, the HighPerformance.UnitTests project must be signed because it's being given friend access via the HighPerformance project.

You can try to build this branch with this instead but you will encounter a few CS1726 build errors.

  <PropertyGroup Condition=" '$(IsCoreProject)' == 'true' ">
    <SignAssembly>true</SignAssembly>
    <AssemblyOriginatorKeyFile>$(RepositoryDirectory)toolkit.snk</AssemblyOriginatorKeyFile>
  </PropertyGroup>

Copy link
Member

@Sergio0694 Sergio0694 left a comment

Choose a reason for hiding this comment

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

Minor nit, other than that LGTM! :shipit:

Thanks!

@hymccord hymccord force-pushed the strongname-sign-assemblies branch from 0af86ce to 376a2e2 Compare June 3, 2022 02:04
@Nirmal4G
Copy link
Contributor

Nirmal4G commented Jun 3, 2022

PSA This is okay as it is but I found this -> dotnet/sdk#3439

FYI We can now add InternalsVisibleTo in the project file!

<!-- Common.props (or in Azure Pipelines) -->
<PublicKey>002400000480000094000000060200000024000052534131000400000100010041753AF735AE6140C9508567666C51C6AB929806ADB0D210694B30AB142A060237BC741F9682E7D8D4310364B4BBA4EE89CC9D3D5CE7E5583587E8EA44DCA09977996582875E71FB54FA7B170798D853D5D8010B07219633BDB761D01AC924DA44576D6180CDCEAE537973982BB461C541541D58417A3794E34F45E6F2D129E2</PublicKey>

<!-- CommunityToolkit.Mvvm -->
<InternalVisibleTo Include="CommunityToolkit.Mvvm.Internals.UnitTests" PublicKey="$(PublicKey)"/>

<!-- CommunityToolkit.HighPerformance -->
<InternalVisibleTo Include="CommunityToolkit.HighPerformance.UnitTests" PublicKey="$(PublicKey)"/>

@hymccord
Copy link
Contributor Author

hymccord commented Jun 3, 2022

I'm aware of the project property. I'm keeping it simple and not modifying more than what is necessary to fix the opened issue.

Simple PRs are less demanding to review.

@Sergio0694 Sergio0694 merged commit a12f5f2 into CommunityToolkit:main Jun 3, 2022
@Sergio0694
Copy link
Member

Thank you @inkahootz! 😄

@hymccord hymccord deleted the strongname-sign-assemblies branch June 3, 2022 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI/pipeline 🔬 Some changes or issues related to CI infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Latest preview assemblies are not signed

4 participants