[libraries] Build lib-runtime-packs for all of mono, not just mobile #35295

mdh1418 · 2020-04-22T16:57:54Z

Fixes #35247
This PR makes a small change to generate our in-tree runtime packs for all of mono, not just iOS, Android, and tvOS.

~~This PR will be updated and merged after #35105 is merged.~~ Rebased ✔️

After the change, lib-runtime-packs is generated in <runtime-repo>/artifacts/bin/ when running ./build.sh -subset mono+libs

Running ./build.sh -subset mono+libs -test -c Release after the change leads to no build errors.

When building for Mono, not mobile (e.g. ./build.sh -subset mono+libs)
We want testhost to be generated under <runtime-root>/artifacts/bin, yet to omit those files from lib-runtime-packs.

After these changes, these 7 files are omitted from lib-runtime-packs

Only in lib-runtime-packs-with-dotnet/runtimes/osx-x64/lib/netcoreapp5.0: dotnet
Only in lib-runtime-packs-with-dotnet/runtimes/osx-x64/lib/netcoreapp5.0: libhostfxr.dylib
Only in lib-runtime-packs-with-dotnet/runtimes/osx-x64/lib/netcoreapp5.0: libhostpolicy.dylib
Only in lib-runtime-packs-with-dotnet/runtimes/osx-x64/lib/netcoreapp5.0: libnethost.a
Only in lib-runtime-packs-with-dotnet/runtimes/osx-x64/lib/netcoreapp5.0: libnethost.dylib
Only in lib-runtime-packs-with-dotnet/runtimes/osx-x64/lib/netcoreapp5.0: nethost.h

ghost · 2020-04-22T16:57:58Z

Tagging subscribers to this area: @ViktorHofer
Notify danmosemsft if you want to be subscribed.

ghost · 2020-04-22T17:30:30Z

Tagging subscribers to this area: @directhex
Notify danmosemsft if you want to be subscribed.

safern · 2020-04-28T18:22:10Z

src/libraries/restore/runtime/runtime.depproj

@@ -50,7 +50,12 @@
      <UseHardlink Condition="'$(_runtimeOSFamily)' == 'FreeBSD'">false</UseHardlink>
    </PropertyGroup>

-    <ItemGroup>
+    <ItemGroup Condition="'$(RuntimeFlavor)' == 'Mono'">
+      <HostFxFile Include="@(StoreTestHostFiles)" Condition="'%(StoreTestHostFiles.Filename)' == '$(HostFxrFileName)'" />


Isn't this going to include dotnet and hostfxr in the runtime packs?

I commented incorrectly the last time, so trying again ;-).

We won't be including dotnet and hostfxr into the runtime packs because we are excluding them from ReferenceLocalCopyPaths and putting them into StoreTestHostFiles. We want to do this because desktop mono is in a state where we need in tree runtime packs, but also need to make sure testhost keeps working (for now).

Last time that I ran the builds for mobile/mono both dotnet and hostfxr were excluded from lib-runtime-packs. When building for mono, dotnet and hostfxr existed in testhost. When building for mobile, neither were in testhost.

When using your suggested changes, the same functionality was achieved.

Ah right. I somehow got confused by the fact that we were copying them over to the test host 😄

src/libraries/restore/runtime/runtime.depproj

safern · 2020-04-28T18:26:47Z

src/libraries/restore/runtime/runtime.depproj

@@ -98,7 +103,11 @@
          DependsOnTargets="ResolveRuntimeFilesFromLocalBuild"
          AfterTargets="AfterResolveReferences;FilterNugetPackages"
          Condition="'$(RuntimeFlavor)' == 'Mono'">
-    <ItemGroup Condition="'$(BinPlaceTestRuntimePack)' != 'true'">
+    <ItemGroup>


Once you do what I suggested here: https://github.com/dotnet/runtime/pull/35295/files#r416829953 You can remove this ItemGroup and then move the TargetsMobile != true condition to the target itself.

Condition="'$(RuntimeFlavor)' == 'Mono' and '$(TargetsMobile)' != 'true'"

I think I might have just broken the tests with this suggestion 😄. Because you're adding RuntimeFiles to ReferenceCopyLocalPaths and then removing them in SetupTestingHost. Let's instead change how these targets are hooked up.

Add DependsOnTargets=OverrideRuntimeMono to SetupTestingHost target and bring back what you had.

Ah okay will change this now. I was wondering if it was breaking because I forgot the '$(RuntimeFlavor)' == 'Mono' check.
Just wondering, what in the build errors in CI would indicate its caused by changes in this PR?

Because the mono tests are crashing because it can’t find some runtime files and your first commit was green.

Something still isn't right. libcoreclr is ending up in lib.

Actually.... I ran the builds before the adjustments, and checked the previous lib-runtime-pack folders. It turns out libcoreclr.dylib was being placed into lib-runtime-packs/runtimes/<rid>/lib/netcoreapp5.0/ when the build was all green too. I didn't catch it prior. ~~Checking right now if mobile also includes that.~~ Mobile didn't include it so it's something from the first 3 commits.

I think the whole ReferenceLocalCopyPaths clearing caused the problem. I threw up a commit where we BinPlace the native libs + runtime into testhost. That basically piggybacks off of what we already have done.

src/libraries/restore/runtime/runtime.depproj

…runtime-packs

safern · 2020-04-29T15:41:48Z

I think this: #35295 (comment) might help you fix the test errors. Sorry for misleading in the first suggestion 😄

…runtime-packs This reverts commit 8cecd66.

…ntime into mdhwang/runtime_pack_all_mono

steveisok · 2020-04-30T11:44:53Z

@safern @mdh1418 All of the files should be going to the right places now.

src/libraries/restore/runtime/runtime.depproj

…PackNative

safern · 2020-04-30T19:53:25Z

src/libraries/Directory.Build.targets

@@ -102,6 +102,12 @@
      <RuntimePath>$(NETCoreAppRuntimePackNativePath)</RuntimePath>
      <ItemName>NativeBinPlaceItem</ItemName>
    </BinPlaceTargetFrameworks>
+    <!-- Corresponding libMono and system.native libraries are added to testhost shared framework -->


@mdh1418 @steveisok I guess this is a workaround until we get the self contained test runs story for Xamarin tests, right?

Yes. It will go away

safern · 2020-04-30T21:44:10Z

src/libraries/Native/native-binplace.proj

@@ -24,6 +24,10 @@
      <BinPlaceItem Condition="'$(BinPlaceTestRuntimePack)' != 'true'" Include="@(NativeBinPlaceItem)" />
      <FileWrites Include="@(NativeBinPlaceItem)" />
    </ItemGroup>
+    <ItemGroup Condition="'$(BinPlaceTestRuntimePack)' == 'true' and '$(TargetsMobile)' != 'true'">
+      <TestHostBinPlaceItem Include="%(NativeBinPlaceItem.Identity)" />


Is this necessary? I think we can achieve the same by updating the condition to include into BinPlaceItem. Something like: '$(BinPlaceTestRuntimePack)' != 'true' or '$(TargetsMobile)' != 'true'?

For my understanding ;-)

Can you give an example of what you think will work?

Instead of adding this new item I think you can just update the above Item to:

Suggested change

<TestHostBinPlaceItem Include="%(NativeBinPlaceItem.Identity)" />

<BinPlaceItem Condition="'$(BinPlaceTestRuntimePack)' != 'true' or '$(TargetsMobile)' != 'true'" Include="@(NativeBinPlaceItem)" />

<FileWrites Include="@(NativeBinPlaceItem)" />

</ItemGroup>

BinPlaceItem should always contribute to the testhost directory. BinPlaceTestRuntimePack will only be true when RuntimeFlavor == Mono, if TargetsMobile != true that means we're building for Xamarin Desktop. Is my assumption right?

Changing the code to

<Target Name="GetBinPlaceItems"> <ItemGroup> <NativeBinPlaceItem Include="$(NativeBinDir)*.dll" /> <NativeBinPlaceItem Include="$(NativeBinDir)*.pdb" /> <NativeBinPlaceItem Include="$(NativeBinDir)*.lib" /> <NativeBinPlaceItem Include="$(NativeBinDir)*.a" /> <NativeBinPlaceItem Include="$(NativeBinDir)*.bc" /> <NativeBinPlaceItem Include="$(NativeBinDir)*.so" /> <NativeBinPlaceItem Include="$(NativeBinDir)*.dbg" /> <NativeBinPlaceItem Include="$(NativeBinDir)*.dylib" /> <NativeBinPlaceItem Include="$(NativeBinDir)*.dwarf" /> <BinPlaceItem Condition="'$(BinPlaceTestRuntimePack)' != 'true' or '$(TargetsMobile)' != 'true'" Include="@(NativeBinPlaceItem)" /> <FileWrites Include="@(NativeBinPlaceItem)" /> </ItemGroup> </Target>

The two testhost, before and after, are identical. However lib-runtime-packs seem to differ, where the one with this change has more files.

Only in lib-runtime-packs/runtimes/osx-x64/lib/netcoreapp5.0: libSystem.Globalization.Native.a Only in lib-runtime-packs/runtimes/osx-x64/lib/netcoreapp5.0: libSystem.Globalization.Native.dylib Only in lib-runtime-packs/runtimes/osx-x64/lib/netcoreapp5.0: libSystem.Globalization.Native.dylib.dwarf Only in lib-runtime-packs/runtimes/osx-x64/lib/netcoreapp5.0: libSystem.IO.Compression.Native.a Only in lib-runtime-packs/runtimes/osx-x64/lib/netcoreapp5.0: libSystem.IO.Compression.Native.dylib Only in lib-runtime-packs/runtimes/osx-x64/lib/netcoreapp5.0: libSystem.IO.Compression.Native.dylib.dwarf Only in lib-runtime-packs/runtimes/osx-x64/lib/netcoreapp5.0: libSystem.IO.Ports.Native.a Only in lib-runtime-packs/runtimes/osx-x64/lib/netcoreapp5.0: libSystem.IO.Ports.Native.dylib Only in lib-runtime-packs/runtimes/osx-x64/lib/netcoreapp5.0: libSystem.IO.Ports.Native.dylib.dwarf Only in lib-runtime-packs/runtimes/osx-x64/lib/netcoreapp5.0: libSystem.Native.a Only in lib-runtime-packs/runtimes/osx-x64/lib/netcoreapp5.0: libSystem.Native.dylib Only in lib-runtime-packs/runtimes/osx-x64/lib/netcoreapp5.0: libSystem.Native.dylib.dwarf Only in lib-runtime-packs/runtimes/osx-x64/lib/netcoreapp5.0: libSystem.Net.Security.Native.a Only in lib-runtime-packs/runtimes/osx-x64/lib/netcoreapp5.0: libSystem.Net.Security.Native.dylib Only in lib-runtime-packs/runtimes/osx-x64/lib/netcoreapp5.0: libSystem.Net.Security.Native.dylib.dwarf Only in lib-runtime-packs/runtimes/osx-x64/lib/netcoreapp5.0: libSystem.Security.Cryptography.Native.Apple.a Only in lib-runtime-packs/runtimes/osx-x64/lib/netcoreapp5.0: libSystem.Security.Cryptography.Native.Apple.dylib Only in lib-runtime-packs/runtimes/osx-x64/lib/netcoreapp5.0: libSystem.Security.Cryptography.Native.Apple.dylib.dwarf Only in lib-runtime-packs/runtimes/osx-x64/lib/netcoreapp5.0: libSystem.Security.Cryptography.Native.OpenSsl.a Only in lib-runtime-packs/runtimes/osx-x64/lib/netcoreapp5.0: libSystem.Security.Cryptography.Native.OpenSsl.dylib Only in lib-runtime-packs/runtimes/osx-x64/lib/netcoreapp5.0: libSystem.Security.Cryptography.Native.OpenSsl.dylib.dwarf

I can run it again, but is this desired?

@steveisok we want the Xamarin Desktop runtime packs to have the native assets produced from libraries, right?

Actually that's wrong. Because it is going under lib. This is not desired. Thanks for checking @mdh1418 -- then we can't use my suggestion here 😄

I answered previously assuming your question. The desktop structure for lib-runtime-packs should mirror mobile. Libs contain the assemblies and native contains the native assets (mono runtime + system.native + SPC).

Right. My suggestion here unfortunately doesn't work 😄. Whenever we start producing self contained assets I guess we can revisit and see if there is a better way.

[libraries] Build lib-runtime-packs for all of mono, not just mobile

69d7d13

Dotnet-GitSync-Bot added the area-Infrastructure label Apr 22, 2020

mdh1418 requested a review from steveisok April 22, 2020 16:58

steveisok added the area-Infrastructure-mono label Apr 22, 2020

steveisok removed the area-Infrastructure label Apr 22, 2020

mdh1418 force-pushed the mdhwang/runtime_pack_all_mono branch from 69d7d13 to 33e2e93 Compare April 22, 2020 21:04

mdh1418 added 2 commits April 23, 2020 09:32

[libraries] Build lib-runtime-packs for all of mono, not just mobile

cc4a3e1

[libraries] Remove dotnet and unnecessary files from lib-runtime-packs

7bb8bca

mdh1418 force-pushed the mdhwang/runtime_pack_all_mono branch from 33e2e93 to 7bb8bca Compare April 23, 2020 14:25

[libraries] Reinclude dotnet and other files into test host

560d208

jaredpar mentioned this pull request Apr 23, 2020

OSX machines are de-provisioned during CI / PR runs leading to failures #34472

Closed

mdh1418 requested review from a team and safern April 27, 2020 20:15

safern reviewed Apr 28, 2020

View reviewed changes

src/libraries/restore/runtime/runtime.depproj Show resolved Hide resolved

safern reviewed Apr 28, 2020

View reviewed changes

src/libraries/restore/runtime/runtime.depproj Outdated Show resolved Hide resolved

[libraries] Alternative to excluding dotnet and other files from lib-…

8cecd66

…runtime-packs

mdh1418 force-pushed the mdhwang/runtime_pack_all_mono branch from 636b4b0 to 8cecd66 Compare April 29, 2020 12:35

mdh1418 and others added 4 commits April 29, 2020 12:00

Undo alternative changes for exclude dotnet and other files from lib-…

08b9b41

…runtime-packs This reverts commit 8cecd66.

Merge branch 'mdhwang/runtime_pack_all_mono' of github.com:mdh1418/ru…

8b1b854

…ntime into mdhwang/runtime_pack_all_mono

BinPlace native libs + runtime into testhost for desktop mono

93080b5

Removed comment

a1897df

safern reviewed Apr 30, 2020

View reviewed changes

src/libraries/restore/runtime/runtime.depproj Outdated Show resolved Hide resolved

[libraries] Remove redundant condition from ItemGroup in SetupRuntime…

d5464fe

…PackNative

safern reviewed Apr 30, 2020

View reviewed changes

safern approved these changes Apr 30, 2020

View reviewed changes

mdh1418 merged commit ac6fac8 into dotnet:master May 1, 2020

mdh1418 deleted the mdhwang/runtime_pack_all_mono branch May 1, 2020 11:58

ghost locked as resolved and limited conversation to collaborators Dec 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libraries] Build lib-runtime-packs for all of mono, not just mobile #35295

[libraries] Build lib-runtime-packs for all of mono, not just mobile #35295

mdh1418 commented Apr 22, 2020 •

edited

Loading

ghost commented Apr 22, 2020

ghost commented Apr 22, 2020

safern Apr 28, 2020

steveisok Apr 28, 2020 •

edited

Loading

mdh1418 Apr 28, 2020

safern Apr 28, 2020

safern Apr 28, 2020

mdh1418 Apr 28, 2020

safern Apr 28, 2020

mdh1418 Apr 29, 2020

safern Apr 29, 2020

steveisok Apr 29, 2020

mdh1418 Apr 30, 2020 •

edited

Loading

steveisok Apr 30, 2020

safern commented Apr 29, 2020

steveisok commented Apr 30, 2020

safern Apr 30, 2020

steveisok Apr 30, 2020

safern Apr 30, 2020

safern Apr 30, 2020

steveisok Apr 30, 2020

safern Apr 30, 2020

mdh1418 Apr 30, 2020

mdh1418 Apr 30, 2020

safern Apr 30, 2020

safern Apr 30, 2020

steveisok Apr 30, 2020

safern Apr 30, 2020

[libraries] Build lib-runtime-packs for all of mono, not just mobile #35295

[libraries] Build lib-runtime-packs for all of mono, not just mobile #35295

Conversation

mdh1418 commented Apr 22, 2020 • edited Loading

ghost commented Apr 22, 2020

ghost commented Apr 22, 2020

Choose a reason for hiding this comment

steveisok Apr 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdh1418 Apr 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

safern commented Apr 29, 2020

steveisok commented Apr 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdh1418 commented Apr 22, 2020 •

edited

Loading

steveisok Apr 28, 2020 •

edited

Loading

mdh1418 Apr 30, 2020 •

edited

Loading