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 Signing in Repos that Override Build Actions #44855

Merged
merged 15 commits into from
Dec 16, 2024

Conversation

ellahathaway
Copy link
Member

@ellahathaway ellahathaway commented Nov 13, 2024

Closes dotnet/source-build#4064

Enables signing for the following repos which were not passing -sign under UB modes:

  • aspnetcore
  • fsharp
  • roslyn

@ellahathaway ellahathaway requested review from a team as code owners November 13, 2024 21:53
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Infrastructure untriaged Request triage from a team member labels Nov 13, 2024
@ellahathaway
Copy link
Member Author

ellahathaway commented Nov 14, 2024

Looks like the sign arg is not valid in fsharp, but I tested passing /p:Sign=true to fsharp and that worked to invoke signing. Updated in 9ef5b62

@ellahathaway
Copy link
Member Author

ellahathaway commented Nov 14, 2024

Signing aspnetcore is failing because there are no ItemsToSign defined in Signing.props: https://github.com/dotnet/aspnetcore/blob/main/eng/Signing.props#L7-L9

Signing aspnetcore artifacts in the VMR needs to be enabled, so I'm continuing this discussion in dotnet/aspnetcore#58445 (comment).

In the meantime, I'm going to draft this PR.

@ellahathaway ellahathaway marked this pull request as draft November 14, 2024 16:25
@ViktorHofer
Copy link
Member

Looks like the sign arg is not valid in fsharp, but I tested passing /p:Sign=true to fsharp and that worked to invoke signing. Updated in 9ef5b62

That's weird. Can you please file an issue in fsharp to track allowing the -sign switch?

@ellahathaway
Copy link
Member Author

Looks like the sign arg is not valid in fsharp, but I tested passing /p:Sign=true to fsharp and that worked to invoke signing. Updated in 9ef5b62

That's weird. Can you please file an issue in fsharp to track allowing the -sign switch?

Yes, filed dotnet/fsharp#18011

@ellahathaway
Copy link
Member Author

Need dotnet/aspnetcore#58987 to unblock aspnetcore signing in this PR.

@ellahathaway
Copy link
Member Author

ellahathaway commented Dec 3, 2024

Interesting. Fsharp is failing on windows with an empty ItemsToSign list. Oddly, it seems like signing is being run before the packages are finished being created. That said, it's a bit hard to tell what's occurring, since I only have access to the log and not the binlog (the log output could be a red herring as it might be a concurrency issue with the printing). I'll continue to investigate this & try to repro locally.

@ViktorHofer
Copy link
Member

FYI, dotnet/source-build#4424 is the reason why the AzDO output isn't really helpful here. Unfortunately, fsharp sometimes returns a 0 exit code even though the inner build failed.

@ellahathaway
Copy link
Member Author

ellahathaway commented Dec 5, 2024

Edited:

Fsharp on window builds VisualFSharp.sln which only produces artifacts when using full framework MSBuild. Since we build with core in UB, there are no artifacts & signing fails with No Items to Sign. The solution here is to allow an empty ItemsToSign list when building VisualFSharp.sln with .NET Core MSbuild.

I included the patch for the fix in a8b7fb5

@ViktorHofer
Copy link
Member

ViktorHofer commented Dec 6, 2024

@ellahathaway are you sure about that? When I look at a linux vertical (example AzureLinux_x64_Cross_x64) I see the following artifacts getting produced:

  New artifact(s) after building fsharp:
    -> FSharp.Compiler.Service/43.9.200-preview.24578.1
    -> FSharp.Core/9.0.200-beta.24578.1
    -> assets/symbols/FSharp.Compiler.Service.43.9.200-preview.24578.1.symbols.nupkg
    -> assets/symbols/FSharp.Core.9.0.200-beta.24578.1.symbols.nupkg

@ellahathaway
Copy link
Member Author

ellahathaway commented Dec 6, 2024

@ellahathaway are you sure about that? When I look at a linux vertical (example AzureLinux_x64_Cross_x64) I see the following artifacts getting produced:

  New artifact(s) after building fsharp:
    -> FSharp.Compiler.Service/43.9.200-preview.24578.1
    -> FSharp.Core/9.0.200-beta.24578.1
    -> assets/symbols/FSharp.Compiler.Service.43.9.200-preview.24578.1.symbols.nupkg
    -> assets/symbols/FSharp.Core.9.0.200-beta.24578.1.symbols.nupkg

Here are the artifacts that I see when I look at the windows vertical:

-   New artifact(s) after building fsharp:
    -> Microsoft.FSharp.Compiler/13.9.200-beta.24578.1
    -> assets/symbols/Microsoft.FSharp.Compiler.13.9.200-beta.24578.1.symbols.nupkg

Diving into the fsharp binlogs, there are the following items being signed in the linux:
image

By contrast, there are the following items being signed in the windows vertical:
image

I filed dotnet/source-build#4799 to investigate the missing package from the Linux build.

@ellahathaway
Copy link
Member Author

ellahathaway commented Dec 6, 2024

It looks like both FSharp.sln and 'VisualFSharp.slndon't produce artifacts when building with .NET Core MSBuild on windows. Passing-noVisualStudio` does not help the signing issue. That said, we may need to keep it regardless because we are not building with visual studio?

I filed dotnet/fsharp#18112 to address the lack of ItemsToSign from the fsharp side. This will have to be addressed first in order to fix the failures in this PR.

@ellahathaway ellahathaway requested a review from mmitche December 16, 2024 16:40
@@ -5,6 +5,7 @@
<BuildScript>$(ProjectDirectory)eng\build$(ShellExtension)</BuildScript>

<BuildActions Condition="'$(DotNetBuildSourceOnly)' != 'true'">$(FlagParameterPrefix)restore $(FlagParameterPrefix)all $(FlagParameterPrefix)pack $(FlagParameterPrefix)publish</BuildActions>
<BuildActions Condition="'$(DotNetBuildSourceOnly)' != 'true' and '$(Sign)' == 'true'">$(BuildActions) $(FlagParameterPrefix)sign</BuildActions>
Copy link
Member

Choose a reason for hiding this comment

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

Is Sign ever true in source build only mode? Does this need the extra conditional?

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, I don't think it'll ever be true. Given that assumption, I don't have a problem with removing the extra conditional.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed in b8dc8d6

Copy link
Member

@mmitche mmitche left a comment

Choose a reason for hiding this comment

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

Approved with the question about SB + Signing

@ViktorHofer
Copy link
Member

It looks like both FSharp.sln and 'VisualFSharp.slndon't produce artifacts when building with .NET Core MSBuild on windows. Passing-noVisualStudio` does not help the signing issue. That said, we may need to keep it regardless because we are not building with visual studio?

Does that mean that we need to switch to desktop msbuild when building fsharp on Windows?

@ellahathaway
Copy link
Member Author

It looks like both FSharp.sln and 'VisualFSharp.slndon't produce artifacts when building with .NET Core MSBuild on windows. Passing-noVisualStudio` does not help the signing issue. That said, we may need to keep it regardless because we are not building with visual studio?

Does that mean that we need to switch to desktop msbuild when building fsharp on Windows?

If we want to produce .vsix files out of the fsharp build, then yes.

@mmitche - pinging for your thoughts on Viktor's question

@ellahathaway ellahathaway enabled auto-merge (squash) December 16, 2024 19:10
@ellahathaway ellahathaway merged commit b5e0903 into dotnet:main Dec 16, 2024
35 of 38 checks passed
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.

Enable signing for arcade repos in the VMR build
3 participants