Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Installer/finalizer in c# #44611

Open
wants to merge 29 commits into
base: main
Choose a base branch
from
Open

Conversation

kasperk81
Copy link
Contributor

@kasperk81 kasperk81 commented Nov 3, 2024

TODOs:
- [ ] get WixToolset.Dtf.WindowsInstaller (https://www.nuget.org/packages/WixToolset.Dtf.WindowsInstaller/4.0.6) uploaded to one of dotnet nuget feed and undo NuGet.config change

  • adjust <FinalizerExe>$(ArtifactsDir)bin/finalizer/$(Architecture)/$(Configuration)/bin/finalizer.exe</FinalizerExe> in GenerateMSIs.targets
  • manual testing

fix #43876

@kasperk81 kasperk81 force-pushed the finalizer-csharp branch 3 times, most recently from 7cabc19 to 8abb521 Compare November 4, 2024 12:05
@joeloff
Copy link
Member

joeloff commented Nov 4, 2024

What is the size of the EXE? We need to ensure that the static SDK does not exceed 250MB - that's historically been a tripwire when we see a noticeable increase in download failures, especially outside North America.

It would be good to ensure that the logging mechanism preserves the structure logging that dutil provides - we chose the C/dutil implementation specifically for size and the logging. The other obstacle is the WiX version. There are very hard requirements about the copy we're using currently and until we wrap up the v5 migration work, we'll probably have to sit on this PR.

As for the pending reboot change - we don't care about what the registry says too much. What the finalizer is doing is passing the exit code from the MSI packages back to the bundle engine if it removed any of them.

Do appreciate your initiative - we've talked about moving this to managed single file executable before

@kasperk81
Copy link
Contributor Author

The AOT-published finalizer.exe binary for win-x64 comes in at 2MB. This setup skips the need for C++ and CMake in dotnet/sdk, making it way easier to debug and maintain the C# code, just like the rest of the repo. Plus, you can use dotnet run for quick local testing, which keeps the workflow nice and simple.

The version limitations are a bit of a drawback, but this is just an initial idea. We can fine-tune it down the line once those version issues are sorted out.

@joeloff
Copy link
Member

joeloff commented Nov 4, 2024

The AOT-published finalizer.exe binary for win-x64 comes in at 2MB. This setup skips the need for C++ and CMake in dotnet/sdk, making it way easier to debug and maintain the C# code, just like the rest of the repo. Plus, you can use dotnet run for quick local testing, which keeps the workflow nice and simple.

The version limitations are a bit of a drawback, but this is just an initial idea. We can fine-tune it down the line once those version issues are sorted out.

This sounds very promising, thank you

@kasperk81
Copy link
Contributor Author

@joeloff if you like to test this branch, download https://www.nuget.org/api/v2/package/WixToolset.Dtf.WindowsInstaller/4.0.6 at c:\temp\packages\ and run .\build.cmd -c Release. it'll be generated at $(ArtifactsDir)bin\finalizer\$(Configuration)\$(SdkTargetFramework)-win-$(Architecture)\publish\finalizer.exe

@marcpopMSFT marcpopMSFT added this to the 10.0.1xx milestone Nov 5, 2024
@marcpopMSFT marcpopMSFT removed the untriaged Request triage from a team member label Nov 5, 2024
@kasperk81 kasperk81 force-pushed the finalizer-csharp branch 3 times, most recently from cbc70e9 to d178e49 Compare November 7, 2024 22:24
@kasperk81 kasperk81 force-pushed the finalizer-csharp branch 2 times, most recently from b25c785 to 7f5d417 Compare November 8, 2024 07:33
@joeloff
Copy link
Member

joeloff commented Nov 12, 2024

@kasperk81 thank you for all the updates. I was out yesterday. Probably one or two more things to address. I'll try and do another pass later tonight

@kasperk81
Copy link
Contributor Author

new error after merge from main

D:\a_work\1\vmr\src\sdk\artifacts\sb\package-cache\microsoft.dotnet.ilcompiler\10.0.0-alpha.1.25060.15\build\Microsoft.NETCore.Native.Publish.targets(80,5): error : Overriding System.Private.CoreLib.dll with a newer version is not supported. Attempted to use D:\a_work\1\vmr\src\sdk\artifacts\sb\package-cache\microsoft.netcore.app.runtime.win-x64\10.0.0-ci\runtimes\win-x64\lib\net10.0\System.Private.CoreLib.dll instead of D:\a_work\1\vmr\src\sdk\artifacts\sb\package-cache\runtime.win-x64.microsoft.dotnet.ilcompiler\10.0.0-alpha.1.25060.15\sdk\System.Private.CoreLib.dll. [D:\a_work\1\vmr\src\sdk\src\Installer\finalizer\finalizer.csproj]

@ViktorHofer any recent work that might explain it?

@ViktorHofer
Copy link
Member

ViktorHofer commented Jan 14, 2025

No idea. Given that this is an ilcompiler error, I would ask @MichalStrehovsky or @jkoritzinsky.

@jkoritzinsky
Copy link
Member

My guess is that now you need a live ILCompiler instead of using the LKG one.

@MichalStrehovsky
Copy link
Member

@ViktorHofer any recent work that might explain it?

This is from dotnet/runtime#109988. Looks like we're in a situation where CoreLib is newer than the compiler.

CoreLib must version with the compiler. These two out of sync would kind of work most of the time, but there would be updates from dotnet/runtime when it would break. So this is a good catch.

We need to use CoreLib that is in sync with the compiler. So either use live compiler, or a runtime pack that is the same version as the LKG compiler.

@jkoritzinsky
Copy link
Member

I've put out a PR at #45973 that should change the SDK and installer builds to use a live ILC (and crossgen2 while I'm at it)

@kasperk81 kasperk81 force-pushed the finalizer-csharp branch 2 times, most recently from 7fa3b43 to 6512457 Compare January 18, 2025 22:16
@kasperk81
Copy link
Contributor Author

I've put out a PR at #45973

@jkoritzinsky that fixed windows build. resolved merge conflict

@joeloff can you run the test?

@kasperk81
Copy link
Contributor Author

ping

@ViktorHofer
Copy link
Member

just FYI, @joeloff is currently unavailable. cc @marcpopMSFT

@nagilson
Copy link
Member

@/joeloff is available now but I think he has a large backlog of stuff

@kasperk81
Copy link
Contributor Author

@jkoritzinsky new error after merging main

src\sdk\src\Installer\finalizer\finalizer.csproj(0,0): error NU1102: (NETCORE_ENGINEERING_TELEMETRY=Restore) Unable to find package Microsoft.NETCore.App.Runtime.win-x64 with version (= 10.0.0-ci)

it was building on Jan 19 #44611 (comment)

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.NET SDK Finalizer Feature Band calculation doesn't match SDK's calculation
9 participants