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

Enable test signing during PR validation jobs #1026

Closed
dagood opened this issue Dec 18, 2019 · 9 comments · Fixed by #31926
Closed

Enable test signing during PR validation jobs #1026

dagood opened this issue Dec 18, 2019 · 9 comments · Fixed by #31926
Assignees
Labels
area-Infrastructure untriaged New issue has not been triaged by the area owner

Comments

@dagood
Copy link
Member

dagood commented Dec 18, 2019

In Core-Setup, PR validation ran test signing. This ensured new changes don't break the signing infrastructure in a fundamental way. It was removed in #1016 to unblock official builds.

# TODO: (Consolidation) Enable test signing during PR validation. https://github.com/dotnet/runtime/issues/1026
#
# CoreCLR only produces the UCRT redist file in Release config. When the redist file isn't
# present, signing fails. For now, only sign in official builds which only run Release mode.
- name: SignType
value: ''

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Dec 18, 2019
@ViktorHofer
Copy link
Member

Why does signing depend on the UCRT redist files?

@dagood
Copy link
Member Author

dagood commented Feb 5, 2020

Someone who knows why CoreCLR needs to sign it would need to answer that. The config in question:

<!-- Sign api-ms-win-core-xstate-l2-1-0 binary as it is only catalog signed in the current SDK. -->
<ItemsToSign
Condition="'$(ConfigurationGroup)' == 'Release' and '$(TargetArchitecture)' == 'x86'"
Include="$(CoreCLRArtifactsPath)Redist\ucrt\DLLs\$(TargetArchitecture)\api-ms-win-core-xstate-l2-1-0.dll" />

@dagood
Copy link
Member Author

dagood commented Feb 5, 2020

I'm not sure what the behavior of that DLL is in general--if it's only packed up in Release builds (so the condition just needs to work right?), if we could enable it in Debug builds, if there's some root issue that could be sorted out.... It's possible it's not hard, I didn't look into it other than filing this tracking issue since it wasn't the priority. Best first step IMO is seeing what happens.

/cc @NikolaMilosavljevic

@ViktorHofer ViktorHofer self-assigned this Feb 5, 2020
@ViktorHofer
Copy link
Member

Thanks, assigning to myself to look further.

@ViktorHofer
Copy link
Member

ViktorHofer commented Feb 5, 2020

cc @janvorli @trylek, why do we only produce that files in Release? Tomas mentioned it might already be obsolete?

@trylek
Copy link
Member

trylek commented Feb 5, 2020

I found a commit that seems related:

dotnet/coreclr@557c8f0

@mmitche - is there any harm in executing the CopyUcrtFiles target even in debug mode so that we can test signing in PR runs?

@trylek
Copy link
Member

trylek commented Feb 7, 2020

@ViktorHofer / @dagood - does there exist any private branch or PR enabling signing in debug builds so that I could give it a try in combination with the fix for CopyUcrtFiles?

@mmitche
Copy link
Member

mmitche commented Feb 7, 2020

@mmitche - is there any harm in executing the CopyUcrtFiles target even in debug mode so that we can test signing in PR runs?

No idea.

@dagood
Copy link
Member Author

dagood commented Feb 7, 2020

does there exist any private branch

Not that I'm aware of. To get started, just changing the lines called out above will kick it off:

- name: SignType
  value: test

@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure untriaged New issue has not been triaged by the area owner
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants