dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.58k stars 10.06k forks source link

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

Open mmitche opened 1 month ago

mmitche commented 1 month ago

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.

dotnet-issue-labeler[bot] commented 1 month ago

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.

dotnet-issue-labeler[bot] commented 1 month ago

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 commented 1 month ago

@ellahathaway

ellahathaway commented 1 month ago

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

wtgodbe commented 1 month ago

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

ellahathaway commented 1 month ago

T-Shirt Size: XS

ellahathaway commented 2 weeks ago

@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 https://github.com/dotnet/sdk/pull/44855 to be blocked.

ViktorHofer commented 2 weeks ago

@ellahathaway any idea why ItemsToSign is empty? It should get filled here: https://github.com/dotnet/aspnetcore/blob/eb68e016a554b4da50d7fb0aeffe897cfabf36c7/eng/Signing.props#L34

wtgodbe commented 2 weeks ago

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

ellahathaway commented 2 weeks ago

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: https://github.com/dotnet/aspnetcore/blob/eb68e016a554b4da50d7fb0aeffe897cfabf36c7/eng/Signing.props#L7-L9

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 commented 2 weeks ago

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

ellahathaway commented 2 weeks ago

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 commented 1 week ago

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 https://github.com/dotnet/source-build/issues/4740

ViktorHofer commented 1 week ago

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 https://github.com/dotnet/dotnet/pull/111 to see how this works today with the -publish build action.

ViktorHofer commented 1 week ago

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 commented 1 week ago

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.