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.

Sign up for GitHub

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

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

Merged
merged 10 commits into from
May 1, 2020
Merged

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

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
69d7d13
[libraries] Build lib-runtime-packs for all of mono, not just mobile
mdh1418 Apr 22, 2020
cc4a3e1
[libraries] Build lib-runtime-packs for all of mono, not just mobile
mdh1418 Apr 23, 2020
7bb8bca
[libraries] Remove dotnet and unnecessary files from lib-runtime-packs
mdh1418 Apr 23, 2020
560d208
[libraries] Reinclude dotnet and other files into test host
mdh1418 Apr 23, 2020
8cecd66
[libraries] Alternative to excluding dotnet and other files from lib-…
mdh1418 Apr 28, 2020
08b9b41
Undo alternative changes for exclude dotnet and other files from lib-…
mdh1418 Apr 29, 2020
8b1b854
Merge branch 'mdhwang/runtime_pack_all_mono' of github.com:mdh1418/ru…
Apr 29, 2020
93080b5
BinPlace native libs + runtime into testhost for desktop mono
Apr 30, 2020
a1897df
Removed comment
Apr 30, 2020
d5464fe
[libraries] Remove redundant condition from ItemGroup in SetupRuntime…
mdh1418 Apr 30, 2020
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
2 changes: 1 addition & 1 deletion src/libraries/Directory.Build.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@
<BinPlaceTestSharedFramework Condition="'$(BuildTargetFramework)' == '$(NetCoreAppCurrent)'">true</BinPlaceTestSharedFramework>
<BinPlaceNETFXRuntime Condition="'$(BuildTargetFramework)' == '$(NetFrameworkCurrent)'">true</BinPlaceNETFXRuntime>

<BinPlaceTestRuntimePack Condition="'$(TargetsMobile)' == 'true'">true</BinPlaceTestRuntimePack>
<BinPlaceTestRuntimePack Condition="'$(RuntimeFlavor)' == 'Mono'">true</BinPlaceTestRuntimePack>

<NETCoreAppTestSharedFrameworkPath>$([MSBuild]::NormalizeDirectory('$(TestHostRootPath)', 'shared', 'Microsoft.NETCore.App', '$(ProductVersion)'))</NETCoreAppTestSharedFrameworkPath>

Expand Down
6 changes: 6 additions & 0 deletions src/libraries/Directory.Build.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,12 @@
<RuntimePath>$(NETCoreAppRuntimePackNativePath)</RuntimePath>
<ItemName>NativeBinPlaceItem</ItemName>
</BinPlaceTargetFrameworks>
<!-- Corresponding libMono and system.native libraries are added to testhost shared framework -->
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Yes. It will go away

Copy link
Member

Choose a reason for hiding this comment

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

Awesome.

<BinPlaceTargetFrameworks Condition="'$(BinPlaceTestRuntimePack)' == 'true' and '$(TargetsMobile)' != 'true'" Include="$(NetCoreAppCurrent)-$(TargetOS)">
<RuntimePath>$(TestHostRuntimePath)</RuntimePath>
<ItemName>TestHostBinPlaceItem</ItemName>
</BinPlaceTargetFrameworks>


<!-- binplace targeting packs which may be different from Build TargetFramework -->
<BinPlaceTargetFrameworks Include="netstandard2.0">
Expand Down
4 changes: 4 additions & 0 deletions src/libraries/Native/native-binplace.proj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)" />
Copy link
Member

Choose a reason for hiding this comment

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

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'?

Copy link
Member

Choose a reason for hiding this comment

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

For my understanding ;-)

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I can run it again, but is this desired?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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 😄

Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

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.

<FileWrites Include="@(TestHostBinPlaceItem)" />
</ItemGroup>
</Target>

<Target Name="Build" DependsOnTargets="BinPlace" />
Expand Down
20 changes: 15 additions & 5 deletions src/libraries/restore/runtime/runtime.depproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
<PackageReference Include="Microsoft.DiaSymReader.Native" Version="1.7.0" />
</ItemGroup>

<ItemGroup Condition="'$(BinPlaceTestRuntimePack)' != 'true'">
<ItemGroup Condition="'$(TargetsMobile)' != 'true'">
<PackageReference Include="Microsoft.NETCore.DotNetHost" Version="$(MicrosoftNETCoreDotNetHostVersion)" />
<PackageReference Include="Microsoft.NETCore.DotNetHostPolicy" Version="$(MicrosoftNETCoreDotNetHostPolicyVersion)" />
<!-- We do not need apphost.exe and the 3.0 SDK will actually remove it.
Expand All @@ -40,6 +40,7 @@
<!-- Setup the testing shared framework host -->
<Target Name="SetupTestingHost"
Condition="'$(BinPlaceTestSharedFramework)' == 'true'"
DependsOnTargets="OverrideRuntimeMono"
AfterTargets="AfterResolveReferences">
<PropertyGroup>
<HostFxrFileName Condition="'$(TargetsWindows)' == 'true'">hostfxr</HostFxrFileName>
Expand All @@ -50,7 +51,12 @@
<UseHardlink Condition="'$(_runtimeOSFamily)' == 'FreeBSD'">false</UseHardlink>
</PropertyGroup>

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

Choose a reason for hiding this comment

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

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

Copy link
Member

@steveisok steveisok Apr 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

<DotnetExe Include="@(StoreTestHostFiles)" Condition="'%(StoreTestHostFiles.Filename)' == 'dotnet'" />
</ItemGroup>

<ItemGroup Condition="'$(RuntimeFlavor)' != 'Mono'">
<HostFxFile Include="@(ReferenceCopyLocalPaths)" Condition="'%(ReferenceCopyLocalPaths.Filename)' == '$(HostFxrFileName)'" />
<DotnetExe Include="@(ReferenceCopyLocalPaths)" Condition="'%(ReferenceCopyLocalPaths.Filename)' == 'dotnet'" />
</ItemGroup>
Expand All @@ -69,7 +75,7 @@
</Target>

<Target Name="SetupRuntimePackNative"
Condition="'$(BinPlaceTestRuntimePack)' == 'true' and '$(RuntimeFlavor)' == 'Mono'"
Condition="'$(RuntimeFlavor)' == 'Mono'"
AfterTargets="AfterResolveReferences">
<ItemGroup>
<NativeBinPlaceItem Include="@(RuntimeFiles)" />
Expand All @@ -80,6 +86,9 @@
<DestinationSubDirectory>include/%(RecursiveDir)</DestinationSubDirectory>
</NativeBinPlaceItem>
</ItemGroup>
<ItemGroup Condition="'$(RuntimeFlavor)' == 'Mono' and '$(TargetsMobile)' != 'true'">
<TestHostBinPlaceItem Include="@(RuntimeFiles)" />
</ItemGroup>
</Target>

<Target Name="OverrideRuntimeCoreCLR"
Expand All @@ -98,8 +107,9 @@
DependsOnTargets="ResolveRuntimeFilesFromLocalBuild"
AfterTargets="AfterResolveReferences;FilterNugetPackages"
Condition="'$(RuntimeFlavor)' == 'Mono'">
<ItemGroup Condition="'$(BinPlaceTestRuntimePack)' != 'true'">
<ReferenceCopyLocalPaths Include="@(RuntimeFiles)" />
<ItemGroup>
Copy link
Member

Choose a reason for hiding this comment

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

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'"

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks!

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 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@mdh1418 mdh1418 Apr 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

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 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.

<StoreTestHostFiles Include="@(ReferenceCopyLocalPaths)" />
<ReferenceCopyLocalPaths Remove="@(ReferenceCopyLocalPaths)" />
</ItemGroup>
</Target>

Expand Down