-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Use the NativeBinary property instead of reconstructing the path in NativeAOT target #117294
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
Conversation
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
This PR adds automatic code signing for native AOT binaries on macOS by introducing an entitlements file and new MSBuild targets.
- Introduce
entitlements.xml
withdisable-library-validation
entitlement - Add
AdHocSign
target inMicrosoft.NETCore.Native.Unix.targets
to codesign the binary after linking - Update
CopyNativeBinary
inMicrosoft.NETCore.Native.Publish.targets
to copy the signed binary
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
File | Description |
---|---|
src/coreclr/nativeaot/BuildIntegration/entitlements.xml | New entitlements file enabling disable-library-validation |
src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.Unix.targets | New AdHocSign MSBuild target for ad-hoc signing on Apple platforms |
src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.Publish.targets | Updated CopyNativeBinary to use the signed $(NativeBinary) |
Comments suppressed due to low confidence (4)
src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.Unix.targets:365
- [nitpick] The target name
AdHocSign
is ambiguous. Consider renaming it toCodesignNativeBinary
to clearly convey its purpose and match existing naming conventions.
<Target Name="AdHocSign"
src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.Unix.targets:365
- [nitpick] It may be helpful to introduce a user-settable property (e.g.,
SkipCodesign
) so teams can opt out of this signing step in certain CI or debug scenarios. You can conditionally run this target based on that property.
<Target Name="AdHocSign"
src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.Publish.targets:92
- [nitpick] Add a comment to explain why we’re now copying the signed
$(NativeBinary)
instead of the original output path to clarify this change for future maintainers.
<Copy SourceFiles="$(NativeBinary)" DestinationFolder="$(PublishDir)" />
src/coreclr/nativeaot/BuildIntegration/entitlements.xml:1
- The entitlements.xml needs the standard plist wrapper and XML declaration to be a valid property list. Consider adding
<?xml version=\"1.0\" encoding=\"UTF-8\"?>
and<plist version=\"1.0\">
before<dict>
, and a closing</plist>
after.
<dict>
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas |
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.
LGTM, thanks!
What is the scenario that this is fixing? |
src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.Unix.targets
Outdated
Show resolved
Hide resolved
src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.Unix.targets
Outdated
Show resolved
Hide resolved
src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.Unix.targets
Outdated
Show resolved
Hide resolved
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.
Turns out this isn't necessary. Native AOT shouldn't need any entitlements. The rest of the cleanup is still nice though.
Reverted the codesign-related changes, left in a small change to use the NativeBinary property instead of reconstructing the path. |
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.
LGTM although I think the title and description are now out of date
Adds a target to codesign the native AOT binary when publishing on a Mac. Assumes no cross-compilation (which we don't support IIRC).Adds entitlements.xml with thedisable-library-validation
entitlement, which appears to be necessary though I haven't determined why yet.Use the NativeBinary property instead of reconstructing the path.