-
Notifications
You must be signed in to change notification settings - Fork 555
[dotnet] Fix support for startup hooks. Fixes #24492. #24664
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
base: main
Are you sure you want to change the base?
Conversation
* Enable support by default, except when Optimize=true (aka Release builds). * Set the STARTUP_HOOKS runtime property if the DOTNET_STARTUP_HOOKS environment variable is set - this is typically done at startup by CoreCLR or MonoVM, but we're using both in an embedding mode, so we have to do it ourselves. * Add a test. Refs: * dotnet/android#10670 * https://github.com/dotnet/runtime/blob/242f7b23752599f22157268de41fee91cb97ef6c/docs/design/features/host-startup-hook.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Fixes Startup Hook support for .NET-on-Apple platforms by enabling it by default for non-optimized builds, explicitly flowing DOTNET_STARTUP_HOOKS into the runtime property bag, and adding a regression test to validate behavior.
Changes:
- Enable
StartupHookSupportby default except whenOptimize=true(typical Release builds). - Pass
STARTUP_HOOKShost property to the embedded runtime whenDOTNET_STARTUP_HOOKSis set. - Add a new
StartupHookTestapp + NUnit test coverage, and extend the test helper to allow asserting non-zero exit codes.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/dotnet/UnitTests/TestBaseClass.cs | Allows asserting an expected (non-zero) exit code when running test apps. |
| tests/dotnet/UnitTests/StartupHookTest.cs | Adds unit tests covering startup hook enabled/disabled behavior across configurations. |
| tests/dotnet/StartupHookTest/AppDelegate.cs | Adds a minimal app that reports whether the startup hook ran. |
| tests/dotnet/StartupHookTest/shared.csproj | Shared project definition for the new startup hook test app. |
| tests/dotnet/StartupHookTest/shared.mk | Shared make configuration to integrate the new test app into existing test infrastructure. |
| tests/dotnet/StartupHookTest/Makefile | Hooks the new test app into the dotnet test make targets. |
| tests/dotnet/StartupHookTest/iOS/StartupHookTest.csproj | iOS target project for the new test app. |
| tests/dotnet/StartupHookTest/iOS/Makefile | iOS make target for the new test app. |
| tests/dotnet/StartupHookTest/tvOS/StartupHookTest.csproj | tvOS target project for the new test app. |
| tests/dotnet/StartupHookTest/tvOS/Makefile | tvOS make target for the new test app. |
| tests/dotnet/StartupHookTest/macOS/StartupHookTest.csproj | macOS target project for the new test app. |
| tests/dotnet/StartupHookTest/macOS/Makefile | macOS make target for the new test app. |
| tests/dotnet/StartupHookTest/MacCatalyst/StartupHookTest.csproj | Mac Catalyst target project for the new test app. |
| tests/dotnet/StartupHookTest/MacCatalyst/Makefile | Mac Catalyst make target for the new test app. |
| runtime/runtime.m | Forwards startup hooks from DOTNET_STARTUP_HOOKS into the runtime host property STARTUP_HOOKS. |
| dotnet/targets/Xamarin.Shared.Sdk.targets | Adjusts default StartupHookSupport behavior and reserves the STARTUP_HOOKS runtimeconfig property. |
jonathanpeppers
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The overall good looks good to me. 👍
But I think you should put the startup hook in a class library for testing.
| <!-- StartupHookSupport is used for Hot Reload, so we enable it unless the Optimize flag is set. --> | ||
| <StartupHookSupport Condition="'$(StartupHookSupport)' == '' And '$(Optimize)' != 'true'">true</StartupHookSupport> | ||
| <StartupHookSupport Condition="'$(StartupHookSupport)' == ''">false</StartupHookSupport> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On Android I also had this in a block for "applications" (that have OutputType=Exe or AndroidApplication=true).
It may not matter, though.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@jonathanpeppers the tests revealed a problem; there's a trimmer warning now for debug builds:
Should we disable startup hooks by default when the trimmer is enabled? AFAIK hot restart isn't compatible with trimming anyway |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@rolfbjarne it will still work when trimmed if you do: <TrimmerRootAssembly Include="StartupHook" RootMode="All" />I did this to verify startup hooks work in NativeAOT, Release mode, etc., but it shows warnings. |
This comment has been minimized.
This comment has been minimized.
Since our device builds are trimmed by default (unless using the interpreter), many projects will trigger the warning now :/ Maybe we could just disable the warning if we enabled StartupHook ourselves. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…upHookSupport=true When we automatically enable StartupHookSupport for Hot Reload (because the developer didn't set it explicitly), suppress the resulting IL2026 trimmer warning from System.StartupHookProvider.ProcessStartupHooks. This is done by setting a feature switch (ObjCRuntime.SuppressStartupHookTrimWarning) that conditionally adds [UnconditionalSuppressMessage] to the method via the linker's link-attributes XML.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
🔥 [CI Build #883f1d7] Test results 🔥Test results❌ Tests failed on VSTS: test results 0 tests crashed, 5 tests failed, 125 tests passed. Failures❌ dotnettests tests (iOS)1 tests failed, 0 tests passed.Failed tests
Html Report (VSDrops) Download ❌ dotnettests tests (MacCatalyst)1 tests failed, 0 tests passed.Failed tests
Html Report (VSDrops) Download ❌ dotnettests tests (macOS)1 tests failed, 0 tests passed.Failed tests
Html Report (VSDrops) Download ❌ dotnettests tests (tvOS)1 tests failed, 0 tests passed.Failed tests
Html Report (VSDrops) Download ❌ monotouch tests (iOS)1 tests failed, 10 tests passed.Failed tests
Html Report (VSDrops) Download Successes✅ cecil: All 1 tests passed. Html Report (VSDrops) Download Pipeline on Agent |
✅ [PR Build #05bb490] Build passed (Detect API changes) ✅Pipeline on Agent |
✅ [CI Build #05bb490] Build passed (Build packages) ✅Pipeline on Agent |
✅ API diff for current PR / commitNET (empty diffs)✅ API diff vs stableNET (empty diffs)ℹ️ Generator diffGenerator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes) Pipeline on Agent |
✅ [CI Build #05bb490] Build passed (Build macOS tests) ✅Pipeline on Agent |
💻 [CI Build #05bb490] Tests on macOS arm64 - Mac Sequoia (15) passed 💻✅ All tests on macOS arm64 - Mac Sequoia (15) passed. Pipeline on Agent |
💻 [CI Build #05bb490] Tests on macOS M1 - Mac Monterey (12) passed 💻✅ All tests on macOS M1 - Mac Monterey (12) passed. Pipeline on Agent |
💻 [CI Build #05bb490] Tests on macOS M1 - Mac Ventura (13) passed 💻✅ All tests on macOS M1 - Mac Ventura (13) passed. Pipeline on Agent |
💻 [CI Build #05bb490] Tests on macOS arm64 - Mac Tahoe (26) passed 💻✅ All tests on macOS arm64 - Mac Tahoe (26) passed. Pipeline on Agent |
💻 [CI Build #05bb490] Tests on macOS X64 - Mac Sonoma (14) passed 💻✅ All tests on macOS X64 - Mac Sonoma (14) passed. Pipeline on Agent |
environment variable is set - this is typically done at startup by CoreCLR
or MonoVM, but we're using both in an embedding mode, so we have to do it
ourselves.
Refs:
$(StartupHookSupport)=false*only* for Release mode android#10670Fixes #24492.