Skip to content

Conversation

@mmitche
Copy link
Member

@mmitche mmitche commented Oct 15, 2024

Add a -sign switch, with dry run signing forced even when the official build ids are set. Enable for some legs in the VMR.

Add a -sign switch, with dry run signing forced even when the official build ids are set. Enable for some legs in the VMR.
@mmitche mmitche requested review from a team as code owners October 15, 2024 21:52
@ghost ghost added Area-Infrastructure untriaged Request triage from a team member labels Oct 15, 2024
@mmitche
Copy link
Member Author

mmitche commented Oct 15, 2024

This will fail until the bootstrap arcade is updated, due to sn signing not being available on non-Windows.

Copy link
Member

@ellahathaway ellahathaway left a comment

Choose a reason for hiding this comment

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

This is a bit orthogonally related to this PR, but one thing I want to clarify is the type of signing that happens when we pass -sign & don't force dry-run signing (in the future).

Because MicroBuild install is not supported when SignType == test on non-windows builds, we cannot do test signing on mac and linux. We should ensure that just because -sign is passed doesn't mean test signing is enabled on non-windows builds. Maybe we continue to force dry-run signing in these scenarios?

@mmitche
Copy link
Member Author

mmitche commented Oct 16, 2024

This is a bit orthogonally related to this PR, but one thing I want to clarify is the type of signing that happens when we pass -sign & don't force dry-run signing (in the future).

Because MicroBuild install is not supported when SignType == test on non-windows builds, we cannot do test signing on mac and linux. We should ensure that just because -sign is passed doesn't mean test signing is enabled on non-windows builds. Maybe we continue to force dry-run signing in these scenarios?

We can add a check in Sign.proj for the SignType. If on non-windows, error in test scenarios. We don't really use test signing all that much, anyway.

targetArchitecture: x64
useDevVersions: true # Use dev versions for CI validation of the experience. If we decide to ship assets from this leg, then we should remove this option.
runTests: false # Temporarily do not run tests. The nuget comparison fails for some non-obvious reason and needs further investigation. Mostly, I'm not sure why it ever passed. https://github.com/dotnet/sdk/issues/42920
sign: true
Copy link
Member

Choose a reason for hiding this comment

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

What's the long-term strategy for this switch? I assume we won't control this per job in the future but per whole build?

Copy link
Member Author

Choose a reason for hiding this comment

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

It will probably still be per job but default to true. We dont' need to sign everything, like PGO.

Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

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

Left one comment (bug around }}] in YML) that needs a fix but otherwise LGTM.

@mmitche mmitche enabled auto-merge (squash) October 17, 2024 17:50
@mmitche mmitche merged commit 73bde76 into dotnet:main Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Infrastructure untriaged Request triage from a team member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants