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

Only reference FreeBSD RIDs when targetting FreeBSD #98948

Merged
merged 1 commit into from
Feb 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/coreclr/tools/aot/ILCompiler/repro/repro.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@
<PlatformTarget>AnyCPU</PlatformTarget>
<AppendTargetFrameworkToOutputPath>false</AppendTargetFrameworkToOutputPath>
<AppendRuntimeIdentifierToOutputPath>false</AppendRuntimeIdentifierToOutputPath>
<RuntimeIdentifiers>linux-x64;win-x64;osx-x64;freebsd-x64;freebsd-arm64</RuntimeIdentifiers>
<RuntimeIdentifiers>linux-x64;win-x64;osx-x64</RuntimeIdentifiers>
<!-- FreeBSD runtime/apphost packs aren't built in the official build so only reference the RIDs when targetting FreeBSD -->
<RuntimeIdentifiers Condition="'$(TargetOS)' == 'freebsd'">$(RuntimeIdentifiers);freebsd-x64;freebsd-arm64</RuntimeIdentifiers>
<Configurations>Debug;Release;Checked</Configurations>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<RunAnalyzers>false</RunAnalyzers>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@
<ArchiveName>dotnet-crossgen2</ArchiveName>
<SharedFrameworkHostFileNameOverride>crossgen2</SharedFrameworkHostFileNameOverride>
<!-- Build this pack for any RID if building from source. Otherwise, only build select RIDs. -->
<RuntimeIdentifiers Condition="'$(DotNetBuildSourceOnly)' != 'true'">linux-x64;linux-musl-x64;linux-arm;linux-musl-arm;linux-arm64;linux-musl-arm64;freebsd-x64;freebsd-arm64;osx-x64;osx-arm64;win-x64;win-x86;win-arm64</RuntimeIdentifiers>
<RuntimeIdentifiers Condition="'$(DotNetBuildSourceOnly)' != 'true'">linux-x64;linux-musl-x64;linux-arm;linux-musl-arm;linux-arm64;linux-musl-arm64;osx-x64;osx-arm64;win-x64;win-x86;win-arm64</RuntimeIdentifiers>
<!-- FreeBSD runtime/apphost packs aren't built in the official build so only reference the RIDs when targetting FreeBSD -->
<RuntimeIdentifiers Condition="'$(DotNetBuildSourceOnly)' != 'true' and '$(TargetOS)' == 'freebsd'">$(RuntimeIdentifiers);freebsd-x64;freebsd-arm64</RuntimeIdentifiers>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<RuntimeIdentifiers Condition="'$(DotNetBuildSourceOnly)' != 'true' and '$(TargetOS)' == 'freebsd'">$(RuntimeIdentifiers);freebsd-x64;freebsd-arm64</RuntimeIdentifiers>
<RuntimeIdentifiers Condition="'$(DotNetBuildSourceOnly)' != 'true' or '$(TargetOS)' == 'freebsd'">$(RuntimeIdentifiers);freebsd-x64;freebsd-arm64</RuntimeIdentifiers>

Is and going to break source build for FreeBSD?

Copy link
Member

Choose a reason for hiding this comment

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

or wouldn't fix the build as this affects non-source build. This should probably just be

Suggested change
<RuntimeIdentifiers Condition="'$(DotNetBuildSourceOnly)' != 'true' and '$(TargetOS)' == 'freebsd'">$(RuntimeIdentifiers);freebsd-x64;freebsd-arm64</RuntimeIdentifiers>
<RuntimeIdentifiers Condition="'$(TargetOS)' == 'freebsd'">$(RuntimeIdentifiers);freebsd-x64;freebsd-arm64</RuntimeIdentifiers>

Copy link
Member Author

Choose a reason for hiding this comment

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

no this should be fine:

  1. source-build case: DotNetBuildSourceOnly == true so this condition doesn't apply now and didn't before
  2. non-source-build case: DotNetBuildSourceOnly == false and now we only add the freebsd RIDs when we're targetting freebsd

Copy link
Member

@jkotas jkotas Feb 26, 2024

Choose a reason for hiding this comment

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

Do we need the RuntimeIdentifiers property at all? What happens if this line is deleted?

These lists of OS/architectures combos are pain to maintain. It would be best to get rid of them where possible.

Copy link
Member

Choose a reason for hiding this comment

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

I think this line defines which crossgen2 packs are getting generated. cc @jkoritzinsky

Copy link
Member

Choose a reason for hiding this comment

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

I'll need to check, but I'm pretty sure we can remove this (as we produce shared framework package per rid-specific build anyway).

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I'll merge this to fix codeflow but I'll open a follow-up PR to try removing the line.

Copy link
Member Author

Choose a reason for hiding this comment

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

<GenerateInstallers>false</GenerateInstallers>
<HostJsonTargetPath>tools/</HostJsonTargetPath>
<PermitDllAndExeFilesLackingFileVersion>true</PermitDllAndExeFilesLackingFileVersion>
Expand Down
Loading