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

[One .NET] Create Host OS specific SDK packs #5245

Merged
merged 1 commit into from
Nov 5, 2020

Conversation

pjcollins
Copy link
Member

@pjcollins pjcollins commented Oct 28, 2020

The Microsoft.Android.Sdk pack has been broken up into three Host OS
packs. Depending on the operating system used during the build, one or
more of the following packs will now be created:

  • Microsoft.Android.Sdk.linux-x64
  • Microsoft.Android.Sdk.osx-x64
  • Microsoft.Android.Sdk.windows-x64

This is still mostly missing Linux support, which can be addressed in
a future PR that adds a linux build job to our commercial pipeline.

I've moved around a few ItemGroups that were used to determine
what ends up in our ref and runtime packs. These were previously
declared in a shared targets file as the assembly content did not differ
between the ref and runtime packs, but now that we are producing
reference assemblies for certain projects we can move the item
declarations into the project files that consume them.

Some duplicate .props/.targets imports have also been cleaned up
throughout build-tools/create-packs/*.

@pjcollins pjcollins marked this pull request as ready for review November 2, 2020 17:52
Copy link
Member

@radekdoulik radekdoulik left a comment

Choose a reason for hiding this comment

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

ILLink related changes LGTM

"version": "@SDK_PACK_VERSION@",
"alias-to": {
"osx-x64": "Microsoft.Android.Sdk.osx-x64",
"windows-x64": "Microsoft.Android.Sdk.windows-x64"
Copy link
Member Author

@pjcollins pjcollins Nov 3, 2020

Choose a reason for hiding this comment

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

We'll need to change this back to win-x64 once dotnet/sdk#14393 lands.

@@ -9,15 +9,14 @@ workload manifest pack containing information about the various Microsoft.Androi
<Project Sdk="Microsoft.Build.NoTargets">

<PropertyGroup>
<TargetFramework>netcoreapp3.1</TargetFramework>
<TargetFramework>net5.0</TargetFramework>
Copy link
Member

Choose a reason for hiding this comment

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

Just an FYI, it seems like it doesn't matter what $(TargetFramework) is set to for a Microsoft.Build.NoTargets project?

https://github.com/microsoft/MSBuildSdks/tree/master/src/NoTargets

$(TargetFramework) just has to be valid and not blank... Maybe there is a way we could this value from a Directory.Build.targets at some point? Then we wouldn't need it in these files.

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 think this is only used when packing, but we ignore it mostly since we set explicit paths for everything in the packs... We can try to centralize it in the future and reduce the warnings coming from our pack invocations at the same time.

Comment on lines 37 to 38
<!-- NOTE: we don't have a reference assembly for Java.Interop.dll yet -->
<_AndroidRefPackAssemblies Include="$(_MonoAndroidNETOutputDir)Java.Interop.dll" />
Copy link
Member

Choose a reason for hiding this comment

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

You might check if we have this file now since we bumped Java.Interop:

dotnet/java-interop@6cde087

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 think we may need to copy it from external/Java.Interop/bin/$(Configuration)-netcoreapp3.1/ref/Java.Interop.dll, it doesn't look like it's automatically copied to the Mono.Android output dir. Maybe there is some msbuild property that controls ref assembly copying for project references that I can find.

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 updated this file to use the external JI path, I couldn't find a built in way to copy this over to the Mono.Android netcore output directory.

@jonpryor
Copy link
Member

jonpryor commented Nov 3, 2020

This is still mostly missing Linux support, which can be addressed in
a future PR that adds a linux build job to our commercial pipeline.

Another benefit to the frequently suggested but never implemented suggestion of moving the proprietary bits out of the "normal" packages is that we won't need a Linux commercial build, as it can use the same proprietary bits as macOS/Windows! (hint hint?)

@@ -34,7 +34,7 @@
<_FilesToCopy Include="$(DotNetPreviewPath)\sdk\$(DotNetPreviewVersionFull)\EnableWorkloadResolver.sentinel" />
<_FilesToCopy Include="$(DotNetPreviewPath)\sdk-manifests\$(DotNetPreviewVersionBand)\Microsoft.NET.Workload.Android\**\*" />
<_FilesToCopy Include="$(DotNetPreviewPath)\packs\Microsoft.Android.Ref\**\*" />
<_FilesToCopy Include="$(DotNetPreviewPath)\packs\Microsoft.Android.Sdk\**\*" />
<_FilesToCopy Include="$(DotNetPreviewPath)\packs\Microsoft.Android.Sdk.osx-x64\**\*" />
Copy link
Member

Choose a reason for hiding this comment

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

This reminds me of a previous conversation around how "safe" it is to use **\* globs, as it could possibly pull in files that we don't expect ("build cruft", files unexpectedly copied from the system mono, etc.).

Should this instead be a more "constrained" glob expression and/or specific files? See e.g. #2733 (comment)

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 wildcard is safe, as all the content in this path was extracted from Microsoft.Android.Sdk.osx-x64.$(Version).nupkg. All of the content being added to the SDK .nupkg file is explicitly defined through https://github.com/xamarin/xamarin-android/blob/master/build-tools/installers/create-installers.targets and https://github.com/xamarin/xamarin-android/blob/master/build-tools/create-packs/Microsoft.Android.Sdk.proj#L71.

This PR did fix one instance of us accidentally including extra build output in our packs by changing the output directory for Microsoft.Android.Sdk.ILLink.

The Microsoft.Android.Sdk pack has been broken up into three Host OS
packs.  Depending on the operating system used during the build, one or
more of the following packs will now be created:

 * Microsoft.Android.Sdk.linux-x64
 * Microsoft.Android.Sdk.osx-x64
 * Microsoft.Android.Sdk.win-x64

This is still mostly missing Linux support, which can be addressed in
a future PR that adds a linux build step to our commercial pipeline.
@jonpryor
Copy link
Member

jonpryor commented Nov 3, 2020

Commit message?

The `Microsoft.Android.Sdk` pack has been broken up into three Host OS
packs.  Depending on the operating system used during the build, one
or more of the following packs will now be created:

  * `Microsoft.Android.Sdk.linux-x64`
  * `Microsoft.Android.Sdk.osx-x64`
  * `Microsoft.Android.Sdk.windows-x64`

This is still mostly missing Linux support, which can be addressed in
a future PR that adds a linux build step to our commercial pipeline.

TODO:

Once we bump to a `dotnet` install which contains
[dotnet/sdk#14393][0], we'll need to replace mentions of
`windows-x64` with `win-x64`.

[0]: https://github.com/dotnet/sdk/pull/14393

@pjcollins pjcollins merged commit 011a771 into dotnet:master Nov 5, 2020
@pjcollins pjcollins deleted the host-os-sdks branch November 5, 2020 17:12
@github-actions github-actions bot locked and limited conversation to collaborators Jan 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants