Skip to content

Create Microsoft.NetCore.App.Ref targeting pack for NetCoreAppCurrent in libraries - #37600

Merged
ViktorHofer merged 9 commits into
dotnet:masterfrom
ViktorHofer:TaretingPackRef
Jun 9, 2020
Merged

Create Microsoft.NetCore.App.Ref targeting pack for NetCoreAppCurrent in libraries#37600
ViktorHofer merged 9 commits into
dotnet:masterfrom
ViktorHofer:TaretingPackRef

Conversation

@ViktorHofer

@ViktorHoferViktorHofer commented Jun 8, 2020•
edited
Loading

Copy link
Copy Markdown
Member

Binplace reference assemblies into a Microsoft.NetCore.App.Ref folder
and also copy data files like the RuntimeList.xml and the
PlatformManifest.txt there.

Rename the runtime pack to microsoft.netcore.app.runtime.$(RID) and always build it.

Fixing a restore issue in the wasm builder (restore and build should
not happen in the same msbuild invocation).

The ref pack will be consumed in #35606.

cc @ericstj

@ViktorHofer
Create Microsoft.NetCore.App.Ref
2151046
Binplace reference assemblies into a Microsoft.NetCore.App.Ref folder
and also copy data files like the RuntimeList.xml and the
PlatformManifest.txt there.
Rename the runtime pack to microsoft.netcore.app.runtime.$(RID)
Fixing a restore issue in the wasm builder (restore and build should
not happen in the same msbuild invocation).
@ghost

ghost commented Jun 8, 2020

Copy link
Copy Markdown

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

@ViktorHoferViktorHofer changed the title Create Microsoft.NetCore.App.RefCreate Microsoft.NetCore.App.Ref targeting pack for NetCoreAppCurrent in librariesJun 8, 2020
akoeplinger
akoeplinger approved these changes Jun 8, 2020
View reviewed changes

@akoeplingerakoeplinger left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

one question, LGTM otherwise

Comment threadsrc/libraries/Directory.Build.props
<MicrosoftNetCoreAppRefPackRefDir>$([MSBuild]::NormalizeDirectory('$(MicrosoftNetCoreAppRefPackDir)', 'ref'))</MicrosoftNetCoreAppRefPackRefDir>
<MicrosoftNetCoreAppRefPackDataDir>$([MSBuild]::NormalizeDirectory('$(MicrosoftNetCoreAppRefPackDir)', 'data'))</MicrosoftNetCoreAppRefPackDataDir>

<MicrosoftNetCoreAppRuntimePackDir>$([MSBuild]::NormalizeDirectory('$(ArtifactsBinDir)', 'microsoft.netcore.app.runtime.$(PackageRID)', '$(Configuration)'))</MicrosoftNetCoreAppRuntimePackDir>

@akoeplingerakoeplingerJun 8, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

could we lift this to some global .props, so we don't need to duplicate it in the iOS/Android/WASM sample .csprojs?

@ViktorHoferViktorHoferJun 8, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@steveisok mentioned that eventually the sample projects will be moved into libraries IIRC

safern
safern reviewed Jun 8, 2020
View reviewed changes
Comment threadsrc/libraries/Directory.Build.targets Outdated
safern
safern reviewed Jun 8, 2020
View reviewed changes
Comment threadsrc/libraries/Native/native-binplace.proj Outdated
safern
safern reviewed Jun 8, 2020
View reviewed changes
Comment threadsrc/mono/netcore/sample/iOS/Program.csproj Outdated
@safern

safern commented Jun 8, 2020

Copy link
Copy Markdown
Member

FYI: @joperezr as this might help you for the linker tests effort.

ericstj
ericstj approved these changes Jun 8, 2020
View reviewed changes

@ericstjericstj left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Cool!

@joperezrjoperezr mentioned this pull request Jun 8, 2020
Merged
4 tasks
joperezr
joperezr reviewed Jun 8, 2020
View reviewed changes
Comment threadsrc/libraries/Directory.Build.props
<MicrosoftNetCoreAppRefPackDataDir>$([MSBuild]::NormalizeDirectory('$(MicrosoftNetCoreAppRefPackDir)', 'data'))</MicrosoftNetCoreAppRefPackDataDir>

<MicrosoftNetCoreAppRuntimePackDir>$([MSBuild]::NormalizeDirectory('$(ArtifactsBinDir)', 'microsoft.netcore.app.runtime.$(PackageRID)', '$(Configuration)'))</MicrosoftNetCoreAppRuntimePackDir>
<MicrosoftNetCoreAppRuntimePackRidDir>$([MSBuild]::NormalizeDirectory('$(MicrosoftNetCoreAppRuntimePackDir)', 'runtimes', '$(PackageRID)'))</MicrosoftNetCoreAppRuntimePackRidDir>

@joperezrjoperezrJun 8, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

NIT: Do we really need to rename all these variables? Seems like the only runtime pack we are producing is for netcoreapp anyways and we are only really using it in very few places so not sure if we are gaining much by making the property names longer. (Of course I specifically ask this because my linker test functionality depends on the property :+1 but I'm happy to change it if the decision is to rename)

@ViktorHoferViktorHoferJun 9, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I prefer an unambiguous meaning as we might introduce a second runtime pack in the future for assets outside of microsoft.netcore.app when we decide to clean the current runtime pack up to only include assets that are part of the shared framework to actually test what we are shipping.

@safern

safern commented Jun 9, 2020

Copy link
Copy Markdown
Member

@ViktorHofer I tried to run iOS and Android tests on this branch to make sure we don't regress them and they both failed to generate the app bundles.

iOS

/Users/santifdezm/repos/runtime/artifacts/bin/System.Buffers.Tests/net5.0-Debug/ios-x64/AppBundle/modules.m:1:10: fatal error: 'mono/jit/jit.h' file not found
#include <mono/jit/jit.h>

It seems like we're missing the include assets in the runtime pack.

Android

/Users/santifdezm/repos/runtime/eng/testing/tests.mobile.targets(36,5): error MSB4018: The "AndroidAppBuilderTask" task failed unexpectedly. [/Users/santifdezm/repos/runtime/src/libraries/System.Buffers/tests/System.Buffers.Tests.csproj]
/Users/santifdezm/repos/runtime/eng/testing/tests.mobile.targets(36,5): error MSB4018: System.ArgumentException: libmonosgen-2.0.a was not found in /Users/santifdezm/repos/runtime/artifacts/bin/System.Buffers.Tests/net5.0-Debug/android-x86/publish/ [/Users/santifdezm/repos/runtime/src/libraries/System.Buffers/tests/System.Buffers.Tests.csproj]

libmonosgen-2.0 is not in the runtime pack so it is missing on the publish dir.

Marking as no merge in the meantime.

@safernsafern added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jun 9, 2020
@ViktorHoferViktorHofer removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jun 9, 2020
@ViktorHofer

ViktorHofer commented Jun 9, 2020

Copy link
Copy Markdown
MemberAuthor

libmonosgen-2.0 is not in the runtime pack so it is missing on the publish dir.

Thanks a lot Santi. Fixed this with 23dfc4e and tested it with Android by following these instructions: https://github.com/dotnet/runtime/blob/master/docs/workflow/testing/libraries/testing-android.md.

@ViktorHofer

ViktorHofer commented Jun 9, 2020

Copy link
Copy Markdown
MemberAuthor

Failure is #37635

@ViktorHofer
ViktorHofer merged commit 2b7852b into dotnet:masterJun 9, 2020
@ViktorHofer
ViktorHofer deleted the TaretingPackRef branch June 9, 2020 10:10
@ghostghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@joperezrjoperezrjoperezr left review comments
@safernsafernsafern left review comments
@akoeplingerakoeplingerakoeplinger approved these changes
@ericstjericstjericstj approved these changes

Assignees

No one assigned

Labels

area-Infrastructure

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@ViktorHofer@safern@akoeplinger@ericstj@joperezr@Dotnet-GitSync-Bot