Build with PublicSign=true by default #99
Conversation
omajid
requested review from
MichaelSimons,
crummel and
lbussell
as code owners
omajid
force-pushed
the
source-build-externals-public-sign
branch
from
a2b0bc5 to
f719b0c
Compare
| <BuildCommandArgs>$(BuildCommandArgs) /v:$(LogVerbosity)</BuildCommandArgs> | ||
| <BuildCommandArgs>$(BuildCommandArgs) $(RedirectRepoOutputToLog)</BuildCommandArgs> | ||
| <BuildCommandArgs>$(BuildCommandArgs) /p:MicrosoftSourceLinkVersion=$(MicrosoftSourceLinkVersion)</BuildCommandArgs> | ||
| <BuildCommandArgs>$(BuildCommandArgs) /p:PublicSign=$(PublicSign)</BuildCommandArgs> |
There was a problem hiding this comment.
Should I also consider passing along other related arcade properties like SignAssemblies?
There was a problem hiding this comment.
@crummel - Can you share your thoughts on this one? I tend to think no.
There was a problem hiding this comment.
No, I think this should be fine without the extra properties.
There was a problem hiding this comment.
Actually taking a second look at this they are pretty tied together so they probably should all be passed, sorry.
There was a problem hiding this comment.
Looking at the properties that are defined at https://github.com/dotnet/arcade/blob/main/src/Microsoft.DotNet.Arcade.Sdk/tools/StrongName.targets:
Writes variables:
DelaySign
PublicSign
PublicKey
PublicKeyToken
AssemblyOriginatorKeyFile
I can see passing DelaySign, PublicSign properties. The other three seem like they would override whatever the public key, token and the snk file that the repo is using. That doesn't seem like a good idea to me. Thoughts?
There was a problem hiding this comment.
Okay, looked at other projects and it seems like they are only passing those 2 properties too (if not just one).
There was a problem hiding this comment.
Yes, I think that makes sense.
omajid
force-pushed
the
source-build-externals-public-sign
branch
from
f719b0c to
82c112c
Compare
omajid
force-pushed
the
source-build-externals-public-sign
branch
2 times, most recently
from
9036407 to
6210397
Compare
repos/newtonsoft-json.proj
Outdated
| <NewtonsoftJsonDirectory>$(ProjectDirectory)/Src/Newtonsoft.Json/</NewtonsoftJsonDirectory> | ||
| <NewtonsoftJsonProjectPath>$(NewtonsoftJsonDirectory)Newtonsoft.Json.csproj</NewtonsoftJsonProjectPath> | ||
| <DotnetToolCommandArguments>"/p:DotnetOnly=true" "/p:Configuration=$(Configuration)" "/p:AssemblyOriginatorKeyFile=$(NewtonsoftJsonKeyFilePath)" "/p:SignAssembly=true" "/p:PublicSign=true" "/p:TreatWarningsAsErrors=false" "/p:AdditionalConstants=SIGNED"</DotnetToolCommandArguments> | ||
| <DotnetToolCommandArguments>"/p:DotnetOnly=true" "/p:Configuration=$(Configuration)" "/p:AssemblyOriginatorKeyFile=$(NewtonsoftJsonKeyFilePath)" "/p:SignAssembly=true" "/p:DelaySign=$(DelaySign)" "/p:PublicSign=true" "/p:TreatWarningsAsErrors=false" "/p:AdditionalConstants=SIGNED"</DotnetToolCommandArguments> |
There was a problem hiding this comment.
I also tried "/p:PublicSign=$(PublicSign)" but that results in a runtime value of /p:PublicSign= being passed, which doesn't work. Any idea why PublicSign is undefined at this point?
There was a problem hiding this comment.
DelaySign is also set by MSBuild's common targets in MS.Common.CurrentVersion.targets: https://github.com/dotnet/msbuild/blob/1a6d753a7648baf0cc991af0c0d254a41d36eba6/src/Tasks/Microsoft.Common.CurrentVersion.targets#L567. PublicSign is not, so I think this is expected. We're passing in the SDK's default value for for DelaySign here instead of Arcade's due to this, but I think that's okay in this case since the rules for what we want to set it to are the same in both cases.
Pass the value of the PublicSign property to individual builds. Arcade sets this property correctly and things just work automagically. Fixes: dotnet/source-build#2907
omajid
force-pushed
the
source-build-externals-public-sign
branch
from
6210397 to
ec936b7
Compare
3 participants
Fixes: dotnet/source-build#2907