Arcade targets override AssemblyOriginatorKeyFile

[sbomer]

When using arcade to publish a project that sets AssemblyOriginatorKeyFile, StrongName.targets overrides the key file (because arcade imports set a default value for StrongNameKeyId):

arcade/src/Microsoft.DotNet.Arcade.Sdk/tools/StrongName.targets

Line 35 in 9800953

<AssemblyOriginatorKeyFile>$(MSBuildThisFileDirectory)snk/35MSSharedLib1024.snk</AssemblyOriginatorKeyFile>

I believe StrongName.targets should not be setting properties as this violates the convention that static properties are computed in props files. Maybe this logic should be moved to StrongName.props, which would allow my project's setting of AssemblyOriginatorKeyFile to take precedence.

I can work around this by:

• setting StrongNameKeyId in my project to some unknown string, preventing arcade's override, and
• setting PublicKey and PublicKeyToken to some unknown string to satisfy the check in

  arcade/src/Microsoft.DotNet.Arcade.Sdk/tools/StrongName.targets

  Lines 84 to 85 in 9800953

  	<Error Condition="'$(SignAssembly)' != 'false' and
  	('$(PublicKey)' == '' or '$(PublicKeyToken)' == '' or '$(AssemblyOriginatorKeyFile)' == '')"

I may have missed something, but I couldn't find any place where PublicKey or PublicKeyToken were actually used, so I think those shouldn't be required to be set in the first place. edit: they're used in the arcade InternalsVisibleTo generator - see below.

[sbomer]

By the same reasoning, it seems like StrongName.targets also shouldn't be overriding DelaySign or PublicSign in

arcade/src/Microsoft.DotNet.Arcade.Sdk/tools/StrongName.targets

Lines 18 to 21 in 9800953

	<PropertyGroup Condition="'$(SignAssembly)' != 'false'">
	<DelaySign>false</DelaySign>
	<PublicSign>true</PublicSign>
	</PropertyGroup>

[tmat]

Maybe this logic should be moved to StrongName.props, which would allow my project's setting of AssemblyOriginatorKeyFile to take precedence.

Why do you need to set AssemblyOriginatorKeyFile in your project? You should be just setting StrongNameKeyId and not to an unknown string but to one of the keys provided by Arcade. Do you need to use a key that's not available in Arcade?

[tmat]

I see you need to use Cecil's key. Does that snk file have public & private key pair?

[tmat]

setting PublicKey and PublicKeyToken to some unknown string

This will break InternalsVisibleTo generator if you use it in your project. I'd recommend you actually setting the correct values for these properties to avoid future surprises that something doesn't work. The checks are there for a reason.

[tmat]

Setting StrongNameKeyId to empty value is ok as long as you set PublicKey, PublicKeyToken and AssemblyOriginatorKeyFile yourself. This means you are not choosing to use any of the Arcade provided keys.

[sbomer]

This will break InternalsVisibleTo generator if you use it in your project.

Thanks, that's what I was missing when I was trying to figure out what these properties were for. We don't use this, but I'll set the PublicKey and PublicKeyToken in case we ever do.

[markwilkie]

@sbomer - anything more to do here?

[sbomer]

@markwilkie, updated the original issue to remove my comment about PublicKey/PublicKeyToken. The rest of it still applies - I believe arcade should be setting these properties in a .props file so that they don't override project files.

[tmat]

I believe arcade should be setting these properties in a .props file so that they don't override project files.

The intent here is to override the project settings if the projects sets StrongNameKeyId (the gotcha is that we set StrongNameKeyId be default, so the project actually need to set it to empty value). Perhaps we could report an error that the project sets both AssemblyOriginatorKeyFile and StrongNameKeyId.
I think we also discussed not setting StrongNameKeyId by default and require the project to set it to some value (including custom which would indicate it uses custom public key, not one that is predefined by Arcade). We can do that as well.

[markwilkie]

Thoughts @sbomer ?

(also cc'ing @JohnTortugo )

[sbomer]

@markwilkie Sorry for the delayed response. My angle is that it should be easy as possible for an existing SDK project to start using arcade (and it definitely shouldn't require digging through arcade targets to come up with a workaround). Maybe that's not really a design goal, in which case feel free to close this.

In my mind StrongNameKeyId is an arcade replacement for AssemblyOriginatorKeyFile that has a bit of extra functionality (automatically sets properties used for the InternalsVisibleTo generator). This replacement should either be opt-in, or have an easy opt-out for projects that want to stick with AssemblyOriginatorKeyFile without using the InternalsVisibleTo generator.

I'm guessing that making it opt-in is more work than it's worth at this point, because presumably lots of arcade projects are already using the default StrongNameKeyId.

So maybe a blend of @tmat's suggestions would provide a nice opt-out. You could set StrongNameKeyId by default as you do today, and support an option (either a 'custom' key id or another property) that doesn't require the project to set PublicKey and PublicKeyToken. Any error should point folks to the right opt-out.

[tmat]

You can set StrongNameKeyId to custom already to opt-out. The one thing that's missing is the error reporting that will tell you to do so.

[tmat]

My angle is that it should be easy as possible for an existing SDK project to start using arcade

That is the goal. :)

[markwilkie]

@sbomer - should I assume you're waiting for error reporting before closing this bug?

[sbomer]

I'm personally hoping for an improved error and the custom opt-out not requiring PublicKey and PublicKeyToken to be set. However it's not blocking anything nor am I actively waiting on it. Triage/close as you see fit. :)

[markwilkie]

Adding to the SDK epic (priority) for post P7