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

Consolidate LocateNativeCompiler target #103375

Merged
merged 4 commits into from
Oct 16, 2024

Conversation

am11
Copy link
Member

@am11 am11 commented Jun 12, 2024

No description provided.

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jun 12, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jun 12, 2024
@am11 am11 added area-NativeAOT-coreclr and removed community-contribution Indicates that the PR has been added by a community member needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jun 12, 2024
@am11 am11 force-pushed the feature/eng/internal-nativeaot-build.targets branch from 908e80a to be8d681 Compare June 13, 2024 07:26
@am11 am11 marked this pull request as ready for review June 13, 2024 07:26
@am11
Copy link
Member Author

am11 commented Jun 13, 2024

Opened #103409, as it's failing on other PRs as well. Rest of the failures are unrelated to PR changes.

@MichalStrehovsky
Copy link
Member

Thanks, this is an awesome improvement, but we need to get official builds stabilized first since we didn't have any for a week. Build work has a high chance of breaking stuff. I'll mark no-merge for now.

@MichalStrehovsky MichalStrehovsky added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jun 14, 2024
Copy link
Member

@sbomer sbomer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once our official builds are unblocked. Thanks a lot!

@@ -23,67 +22,7 @@
<PackageReference Include="runtime.$(ToolsRID).Microsoft.DotNet.ILCompiler" Version="$(MicrosoftDotNetILCompilerVersion)" />
</ItemGroup>

<!-- Needed for the amd64 -> amd64 musl cross-build to pass the target flag. -->
<Target Name="_FixIlcTargetTriple"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know why this is no longer needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be handled by

<CrossCompileAbi Condition="$(CrossCompileRid.StartsWith('linux-musl-')) or $(CrossCompileRid.StartsWith('alpine-'))">musl</CrossCompileAbi>
(since it's required by public consumption as well)

@am11
Copy link
Member Author

am11 commented Jun 25, 2024

Failures are unrelated according to Build Analysis. I think it would be good to run outerloop nativeaot leg against this PR to rule out any latent surprises.

@am11 am11 added community-contribution Indicates that the PR has been added by a community member and removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) blocked Issue/PR is blocked on something - see comments labels Oct 12, 2024
@am11 am11 force-pushed the feature/eng/internal-nativeaot-build.targets branch from be64d07 to cb3e150 Compare October 12, 2024 21:19
@jkotas
Copy link
Member

jkotas commented Oct 13, 2024

/azp run runtime-coreclr crossgen2 outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@sbomer Could you please review this as well? Most of the changes are in the trimming tests.

@jkotas jkotas requested a review from sbomer October 13, 2024 06:50
@am11 am11 force-pushed the feature/eng/internal-nativeaot-build.targets branch from d50b9b6 to 04f086a Compare October 13, 2024 16:40
Comment on lines -14 to -20
<PropertyGroup>
<!-- in net9.0 we can do this, but only on mobile apple platforms, not OSX -->
<SharedLibraryInstallName>@rpath/$(MSBuildProjectName).dylib</SharedLibraryInstallName>
</PropertyGroup>
<ItemGroup Condition="'$(TargetsOSX)' == 'true'">
<LinkerArg Include="-Wl,-install_name,@rpath/$(MSBuildProjectName).dylib" />
</ItemGroup>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We generally need to specify the @rpath/-prefixed name on macOS for libraries. Is that done somewhere else now? What is the output for otool -l before and after?

Copy link
Member Author

@am11 am11 Oct 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that done somewhere else now?

Seems to me like an avoidable duplicate:

<LinkerArg Include="-Wl,-install_name,&quot;$(SharedLibraryInstallName)&quot;" Condition="'$(_IsiOSLikePlatform)' == 'true' and '$(NativeLib)' == 'Shared'" />

Can we use it for all apple platforms? Or are there use-cases when we explicitly don't need it?

Copy link
Member

@filipnavara filipnavara Oct 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use it for all apple platforms? Or are there use-cases when we explicitly don't need it?

I'd say we can. We would need to check with Xamarin workloads to ensure that it doesn't break anything.

Note that generally Xamarin workloads have an SDK code on the consumption side where it fixes up the paths with install_name_tool if they are wrong, so I don't expect it to break anything there. I am more concerned that the workload may also specify -install_name on the library compilation side and it would collide if we add it in the NativeAOT linker args.

Copy link
Member Author

@am11 am11 Oct 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With 9a97e5e the behavior is:

-      cmdsize 72
-         name bin/Release/net9.0/osx-arm64/native/helloworld.dylib (offset 24)
+      cmdsize 48
+         name @rpath/helloworld.dylib (offset 24)
  • User can skip setting install_name with SkipInstallName
  • If user has <LinkerArg Include="-Wl,-install_name,@rpath/TheirName" /> type of thing specified in their project, TheirName takes precedence with SkipInstallName=true

What do you think?

cDAC (the project being modified) is currently only used for testing in the repo, which is working, so I think we can live with it without waiting for next SDK update which won't be long (10.0 alpha1).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jkotas jkotas merged commit c627f38 into dotnet:main Oct 16, 2024
147 of 149 checks passed
@am11 am11 deleted the feature/eng/internal-nativeaot-build.targets branch October 16, 2024 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-NativeAOT-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants