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 arcade -sign functionality across aspnetcore's build #58445

Open
Tracked by #3708
mmitche opened this issue Oct 15, 2024 · 18 comments · May be fixed by #59590
Open
Tracked by #3708

Enable arcade -sign functionality across aspnetcore's build #58445

mmitche opened this issue Oct 15, 2024 · 18 comments · May be fixed by #59590
Assignees
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework task

Comments

@mmitche
Copy link
Member

mmitche commented Oct 15, 2024

aspnetcore's signing functionality depends somewhat on shuttling bits from Linux/Mac to Windows. As arcade gets proper support for -sign on Mac and Linux, we should alter aspnetcore to be more 'vertical'. No shuttling around. This has benefits for aspnetcore in the short term, allows arcade to prove out its non-Windows signing functionality, and sets up aspnetcore builds to work well in signed VMR builds.

Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

1 similar comment
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@mmitche mmitche transferred this issue from dotnet/source-build Oct 15, 2024
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Oct 15, 2024
@mmitche mmitche changed the title Enable arcade -sign functionality in aspnetcore's build Enable arcade -sign functionality across aspnetcore's build Oct 15, 2024
@mmitche
Copy link
Member Author

mmitche commented Oct 15, 2024

@ellahathaway

@ellahathaway
Copy link
Member

FYI this is currently blocked on a MicroBuild issue: https://dev.azure.com/devdiv/DevDiv/_workitems/edit/2270151

@wtgodbe
Copy link
Member

wtgodbe commented Oct 22, 2024

Awesome! Let me know if there's anything I can do to help

@mkArtakMSFT mkArtakMSFT added this to the .NET 10 Planning milestone Oct 29, 2024
@ellahathaway
Copy link
Member

T-Shirt Size: XS

@ellahathaway
Copy link
Member

@wtgodbe - Is there a way to make progress on this issue without enabling arcade signing in aspnetcore's Mac & Linux legs, which is currently blocked on https://dev.azure.com/devdiv/DevDiv/_workitems/edit/2270151? The lack of ItemsToSign (removed here) is causing my changes in dotnet/sdk#44855 to be blocked.

@ViktorHofer
Copy link
Member

@ellahathaway any idea why ItemsToSign is empty? It should get filled here:

<ItemsToSign Include="@(CommonFilesToSign)" />

@wtgodbe
Copy link
Member

wtgodbe commented Nov 14, 2024

@ellahathaway I think the link at "removed here" is wrong, it just links to this issue

@ellahathaway
Copy link
Member

ellahathaway commented Nov 14, 2024

any idea why ItemsToSign is empty? It should get filled here:

I'm investigating. It's weird because the binlog does not show the ItemsToSign ever being re-added, and it does not list CommonFilesToSign as defined either. Oddly it does show other items being evaluated, such as the FileExtensionSignInfos

I think the link at "removed here" is wrong, it just links to this issue

Whoops sorry. Should be this link:

<ItemsToSign Remove="@(ItemsToSign)" />
<ItemsToSignPostBuild Remove="@(ItemsToSignPostBuild)" />
<FileExtensionSignInfo Remove="@(FileExtensionSignInfo)" />

Oooh maybe it's the PostBuildSign property. I don't see it being set in the binlog, so it's possible that that's resulting in both when conditions not being evaluated.

@ViktorHofer
Copy link
Member

Yes, the VMR build shouldn't do post-build signing. The property shouldn't be set.

@ellahathaway
Copy link
Member

ellahathaway commented Nov 16, 2024

I looked into this a bit further, and it appears that there's no artifacts that match the patterns for ItemsToSign.

I would expect to see .nupkgs in D:\a\_work\1\vmr\src\aspnetcore\artifacts\packages\Release\, but I don't see any listed in the binlogs or in the .log file. For example, I don't see something like Successfully created package '/__w/1/s/artifacts/sb/src/artifacts/packages/Release/Shipping/Microsoft.AspNetCore.App.Runtime.<os>-<arch>.10.0.0-ci.nupkg., but I don't.

This is taken from the source-build binlog from the failing UB PR, and these are the only matches for nupkg in the binlog:
Image

@ViktorHofer
Copy link
Member

I looked into this a little bit and noticed that aspnetcore has two inner builds (native and managed) which write to the same binlog output path. Filed dotnet/source-build#4740

  • Build.native.binlog: The native (desktop msbuild) outer repo binlog
  • source-inner-build.binlog: Initially the native (desktop msbuild) inner repo binlog but gets overwritten if the managed inner build gets invoked. If the native part already fails then it doesn't get overwritten.
  • Build.binlog: The managed (dotnet build)

@ViktorHofer
Copy link
Member

ViktorHofer commented Nov 18, 2024

Yes, the item group is empty but for the native build. The managed build didn't even run yet because the native one failed because it doesn't find any artifacts to sign (which I think is expected). Submitted dotnet/dotnet#111 to see how this works today with the -publish build action.

@ViktorHofer
Copy link
Member

OK so the issue here is that aspnetcore's build is basically a two-phase build but both phases receive the -sign and -publish arguments and I'm not exactly sure that's what we want. Additionally, it looks like not just the binlog from the native phase is overwritten but also the asset manifest. So if the native part would actually publish anything, it wouldn't be respected.

I considered multiple options (i.e. just allowing an empty sign list) but really this needs to be fixed at the invocation layer so that aspnetcore doesn't attempt to sign and publish twice. Honestly, aspnetcore's two phase build feels quite hacky to me. Other repos solve this differently.

@mmitche
Copy link
Member Author

mmitche commented Nov 18, 2024

I considered multiple options (i.e. just allowing an empty sign list) but really this needs to be fixed at the invocation layer so that aspnetcore doesn't attempt to sign and publish twice. Honestly, aspnetcore's two phase build feels quite hacky to me. Other repos solve this differently.

The use of vcxproj makes it tough. I think that still requires desktop at this point.

I would go with passing -sign to only certain phases for now.

@ellahathaway
Copy link
Member

Blocked on dotnet/source-build#4794

@ellahathaway ellahathaway moved this to Blocked in .NET Unified Build Dec 5, 2024
@ellahathaway
Copy link
Member

The blocking issue has now been resolved. Moving this back to In Progress

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework task
Projects
Status: In Progress
Development

Successfully merging a pull request may close this issue.

5 participants