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

[structure] Rework installation directory structure #704

Merged
merged 1 commit into from
Jul 28, 2017

Conversation

jonpryor
Copy link
Member

@jonpryor jonpryor commented Jul 26, 2017

Context: #253 (comment)

The intention is that Jenkins-produced oss-xamarin.android*.zip
artifacts be usable on Windows, so that side-by-side testing can be
perfomed without replacing the system installation. Usage is in
UsingJenkinsBuildArtifacts.md.

This isn't entirely the case. It was apparently the case at the
time of commit 87ca273, but things have bitrotten since, and/or the
support was always insufficient.

For example, 87ca273 states that it should be possible to use
msbuild /t:TargetFrameworkRootPath=... to specify the
lib\xbuild-frameworks directory as the location to find
Mono.Android framework assemblies.

...and this works! So long as your project only uses Xamarin.Android
assemblies and projects.

PCL assemblies and projects do not fall under this umbrella.
Consequently, attempting to use PCL assemblies while overriding
$(TargetFrameworkRootPath) results in a gnarly error:

    error MSB3644: The reference assemblies for framework ".NETPortable,Version=v4.5,Profile=Profile259" were not found.

This significantly reduces the usefulness of
oss-xamarin.android*.zip on Windows. Fix this by altering the
_GetReferenceAssemblyPaths target to use
$(XATargetFrameworkRootPath), not $(TargetFrameworkRootPath).
This removes the need to override $(TargetFrameworkRootPath).

Additionally, PR #253 mentions that, for filesystem organization, it
would be useful if the macOS/Linux directory structure --
$prefix/bin, $prefix/lib/mandroid,
$prefix/lib/xbuild/Xamarin/Android, $prefix/lib/xbuild-frameworks
-- more closely resembled the Windows directory structure of
$MSBuild and $ReferenceAssemblies (as seen in .vsix files).
This would turn macOS/Linux into using $xa_prefix/xbuild and
$xa_prefix/xbuild-frameworks directories.

$prefix/bin would only contain xabuild. What is currently in
$prefix/lib/mandroid would be merged with
$xa_prefix/xbuild/Xamarin/Android.

$xa_prefix, meanwhile, could become
$prefix/lib/oss-xamarin.android.

This would turn the current macOS structure:

    $prefix/bin/xabuild
    $prefix/bin/generator
    $prefix/bin/cross-arm
    $prefix/lib/mandroid/generator.exe
    $prefix/lib/xbuild-frameworks/MonoAndroid/v1.0/mscorlib.dll
    $prefix/lib/xbuild/Xamarin/Android/Xamarin.Android.Common.targets

Into:

    $prefix/bin/xabuild
    $prefix/lib/xamarin.android/xbuild-frameworks/MonoAndroid/v1.0/mscorlib.dll
    $prefix/lib/xamarin.android/xbuild/Xamarin/Android/generator.exe
    $prefix/lib/xamarin.android/xbuild/Xamarin/Android/Xamarin.Android.Common.targets
    $prefix/lib/xamarin.android/xbuild/Xamarin/Android/Darwin/cross-arm

Other notes:

  • The bundle-*.zip filename has been has been changed to include a
    hash of the contents of various files, in particular
    build-tools\mono-runtimes.*. This was instigated via a
    conversation with @kumpera about making the filename more
    "idiot-proof": mono-runtimes.props contains compiler flags,
    and if any of those change, then logically the bundle should
    differ as well, as the mono runtimes may differ in significant
    ways. In theory, the -v18 version should be used to track this,
    but this is a manual change, easy to overlook. The new -hHASH
    part of the filename should be more automagic.

    The new <HashFileContents/> task in xa-prep-tasks.dll is
    responsible for creating the hash value.

  • Configuration.Java.Interop.Override.props was moved into
    build-tools/scripts, becauase that would cleanup the root
    directory a bit.

  • OS-specific binaries are now placed into
    $prefix/lib/xamarin.android/xbuild/Xamarin/Android/$(HostOS).
    On a theoretical plus side, this means that the same build
    directory can contain OS-specific binaries for multiple operating
    systems. (I don't know if anyone shares a build tree between e.g.
    macOS and Linux, but if anyone does...)

  • $(MonoAndroidToolsDirectory) should be considered dead, along
    with $(MonoAndroidBinDirectory), as these should now always
    be the same directory as where Xamarin.Android.Build.Tasks.dll
    is located, or a $(HostOS) sub-directory.

  • Xamarin.ProjectTools.csproj needed to be updated to ensure that
    the build order was correct.

  • Remove all [Obsolete] and unreferenced members from
    Xamarin.Android.Build.Utilities.dll. There's too much in there,
    and it makes my head hurt trying to understand the
    interrelationships between it all. If it's not used, it's gone.

  • Work around an xbuild-ism: %(_LlvmRuntime.InstallPath) and
    %(_MonoCrossRuntime.InstallPath) cannot use MSBuild
    properties, e.g. <InstallPath>$(HostOS)/</InstallPath> doesn't
    work as desired; it's instead treated literally.
    Special-case %(InstallPath) until we fully migrate to MSBuild.

@jonpryor jonpryor added the do-not-merge PR should not be merged. label Jul 26, 2017
@jonpryor jonpryor requested a review from dellis1972 July 26, 2017 20:34
@jonpryor jonpryor force-pushed the jonp-bin-structure branch 4 times, most recently from 2c3000c to 08dfd40 Compare July 27, 2017 01:26
@@ -147,7 +147,7 @@ protected override string GenerateCommandLineCommands ()
}

protected override string ToolName {
get { return OS.IsWindows ? "generator.exe" : "generator"; }
get { return "generator.exe"; }
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume the the ToolTask will automatically append "mono" when executing this?

Copy link
Member Author

Choose a reason for hiding this comment

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

ToolTask uses System.Diagnostics.Process, which checks to see if ProcessStartInfo.FileName is a managed exe. If it is, the current mono is used.

Meaning so long as we use Process to create the process, we don't need a wrapper script. It Just Works.

(Which is why we still need the mono-symbolicate script, as that's not done through Process but through <Exec/>, which doesn't have the special-cased managed code logic.)

@@ -21,7 +21,7 @@ public class MDoc : ToolTask
public bool RunExport { get; set; }

protected override string ToolName {
get { return OS.IsWindows ? "mdoc.exe" : "mdoc"; }
get { return "mdoc.exe"; }
Copy link
Contributor

Choose a reason for hiding this comment

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

same here.

os = e.Data.Trim ();
};
DataReceivedEventHandler error = (o, e) => {};
int r = RunProcess ("uname", "-s", output, error);
Copy link
Contributor

Choose a reason for hiding this comment

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

How is this going to work on windows? Is the expectation that it will just return ""? Based on the Path.DirectorySeparatorChar check?
Not sure I like the Path.DirectorySeparatorChar I would rather have a proper call to check the Environment Platform instead as its more obvious what is going on.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, based on the Path.DirectorySeparatorChar check. That could be OS.IsWindows if you prefer.

@@ -9,7 +9,7 @@
<RootNamespace>Xamarin.Android.Tasks</RootNamespace>
<AssemblyName>Xamarin.Android.Build.Tasks</AssemblyName>
<FileAlignment>512</FileAlignment>
<TargetFrameworkVersion>v4.5</TargetFrameworkVersion>
<TargetFrameworkVersion>v4.5.1</TargetFrameworkVersion>
Copy link
Contributor

Choose a reason for hiding this comment

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

Not 100% sure this won't break VS2012 since I'm not sure if that IDE can load v4.5.1 assemblies (afaik VS2012 was released before v4.5.1 was out... I'm happy to be wrong though :) ). It will need testing.

Copy link
Member Author

Choose a reason for hiding this comment

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

The whole reason for this change is because Xamarin Studio/Visual Studio for Mac refused to properly build this project, because one of the projects it referenced (libZipSharp?) had a $(TargetFrameworkVersion) of v4.5.1.

Which is funny, because it built Just Fine™ from the command-line... :-/

We could remove this line, if necessary, but it would be nice to be able to load/build from XS...

@@ -583,7 +583,7 @@ Copyright (C) 2011-2012 Xamarin. All rights reserved.
<Target Name="_GetReferenceAssemblyPaths">
<GetReferenceAssemblyPaths
TargetFrameworkMoniker="$(TargetFrameworkIdentifier),Version=v1.0"
RootPath="$(TargetFrameworkRootPath)">
RootPath="$(XATargetFrameworkRootPath)">
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is the value for $(XATargetFrameworkRootPath) defined? I can't see it in x-a or monodroid. How does GetReferenceAssemblyPaths behave if that is empty? Should it have a default value of $(TargetFrameworkRootPath) if it is not set?

Copy link
Member Author

Choose a reason for hiding this comment

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

Where is the value for $(XATargetFrameworkRootPath) defined?

Where is the value for $(TargetFrameworkRootPath) defined? It isn't, not in normal customer projects. So normally this passes an empty string, which causes <GetReferenceAssemblyPaths/> to search its global directories.

Overriding $(TargetFrameworkRootPath) is documented in UsingJenkinsBuildArtifacts.md, and exists so that our "side-loaded install" is used instead of any existing system-wide install. The problem is that if you actually do override $(TargetFrameworkRootPath), it impacts everything, as mentioned in the commit message.

Primary use remains the same: by default $(XATargetFrameworkRootPath) will be unset, causing <GetReferenceAssemblyPaths/> to search global directories. By using a different property name, we don't clobber/destroy PCL lookup.

Copy link
Member Author

Choose a reason for hiding this comment

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

...except this isn't entirely accurate: the Linux PR #726 build failure is because we're not overriding $(TargetFrameworkRootPath):

Target GetReferenceAssemblyPaths:
Task "CreateProperty"
        Using task CreateProperty from Microsoft.Build.Tasks.CreateProperty, Microsoft.Build.Tasks.Core, Version=14.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a
Done executing task "CreateProperty"
Task "GetReferenceAssemblyPaths"
        Using task GetReferenceAssemblyPaths from Microsoft.Build.Tasks.GetReferenceAssemblyPaths, Microsoft.Build.Tasks.Core, Version=14.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a
        Looking for framework 'MonoAndroid,Version=v7.1' in root path '/usr/lib/mono/4.5/../xbuild-frameworks'
        Unable to find framework definition file '/usr/lib/mono/4.5/../xbuild-frameworks/MonoAndroid/v7.1/RedistList/FrameworkList.xml' for Target Framework Moniker 'MonoAndroid,Version=v7.1'
Microsoft.Common.targets:  warning : Unable to find framework corresponding to the target framework moniker 'MonoAndroid,Version=v7.1'. Framework assembly references will be resolved from the GAC, which might not be the intended behavior.
Done executing task "GetReferenceAssemblyPaths"
Done building target "GetReferenceAssemblyPaths" in project "/mnt/jenkins/workspace/xamarin-anroid-linux-pr-builder/xamarin-android/src/Xamarin.Android.NUnitLite/Xamarin.Android.NUnitLite.csproj".

That raises an interesting conundrum: it might not be possible to NOT override $(TargetFrameworkRootPath), which means the only way to "sanely" build on Windows is by "installing" it.

😢


// I can never remember the difference between SdkPath and anything else...
[Obsolete ("Do not use.")]
public string SdkPath { get; private set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

A note of all the removals here. Doing this we break API compatibility with android-tools. Should these changes been pushed to the xamarin-android-tools repo?
AFAIK the end goal is to move all the SDK resolving code to xamarin-android-tools and have Xamarin.Android.Build.Utilities and android-tools use that so we have one common code base.

Copy link
Member Author

Choose a reason for hiding this comment

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

Doing this we break API compatibility with android-tools.

Do we need to maintain API compatibility? That's a separate repo, and will continue to exist.

Should these changes been pushed to the xamarin-android-tools repo?

Probably. :-/

AFAIK the end goal is to move all the SDK resolving code to xamarin-android-tools and have Xamarin.Android.Build.Utilities and android-tools use that so we have one common code base.

That is and continues to be an end goal.

What we don't yet know is what that end goal even looks like.

For example, in the New World Order this PR moves us toward, there is no longer a reason for most of MonoDroidSdk. What's there is logic to find the XA profile assemblies based on "runtimePath", which is completely redundant, because that's what the bclPath parameter to MonoDroidSdk.Refresh() provides.

The only reason it isn't any smaller is because we still need the logic to know which Android API levels we support -- MonoDroidSdk.GetFrameworkVersionForApiLevel() and related -- and that is better handled through PR #599 (once it actually works).

Meaning the end goal could be the complete removal of MonoDroidSdk and related types, because we don't need them anymore.

Is that not a wonderful state of affairs?

@jonpryor jonpryor force-pushed the jonp-bin-structure branch 4 times, most recently from a2f2655 to 1de3e3b Compare July 27, 2017 21:22
Context: dotnet#253 (comment)

The *intention* is that Jenkins-produced `oss-xamarin.android*.zip`
artifacts be usable on Windows, so that side-by-side testing can be
perfomed without replacing the system installation. Usage is in
[UsingJenkinsBuildArtifacts.md](Documentation/UsingJenkinsBuildArtifacts).

This isn't *entirely* the case. It was *apparently* the case at the
time of commit 87ca273, but things have bitrotten since, and/or the
support was always insufficient.

For example, 87ca273 states that it should be possible to use
`msbuild /t:TargetFrameworkRootPath=...` to specify the
`lib\xbuild-frameworks` directory as the location to find
`Mono.Android` framework assemblies.

...and this works! So long as your project *only* uses Xamarin.Android
assemblies and projects.

PCL assemblies and projects do *not* fall under this umbrella.
Consequently, attempting to use PCL assemblies while overriding
`$(TargetFrameworkRootPath)` results in a gnarly error:

	error MSB3644: The reference assemblies for framework ".NETPortable,Version=v4.5,Profile=Profile259" were not found.

This significantly reduces the usefulness of
`oss-xamarin.android*.zip` on Windows. Fix this by TODO ARGH.

Additionally, PR dotnet#253 mentions that, for filesystem organization, it
would be useful if the macOS/Linux directory structure --
`$prefix/bin`, `$prefix/lib/mandroid`,
`$prefix/lib/xbuild/Xamarin/Android`, `$prefix/lib/xbuild-frameworks`
-- more closely resembled the Windows directory structure of
`$MSBuild` and `$ReferenceAssemblies` (as seen in `.vsix` files).
This would turn macOS/Linux into using `$xa_prefix/xbuild` and
`$xa_prefix/xbuild-frameworks` directories.

`$prefix/bin` would only contain `xabuild`. What is currently in
`$prefix/lib/mandroid` would be merged with
`$xa_prefix/xbuild/Xamarin/Android`.

`$xa_prefix`, meanwhile, could become
`$prefix/lib/oss-xamarin.android`.

This would turn the current macOS structure:

	$prefix/bin/xabuild
	$prefix/bin/generator
	$prefix/bin/cross-arm
	$prefix/lib/mandroid/generator.exe
	$prefix/lib/xbuild-frameworks/MonoAndroid/v1.0/mscorlib.dll
	$prefix/lib/xbuild/Xamarin/Android/Xamarin.Android.Common.targets

Into:

	$prefix/bin/xabuild
	$prefix/lib/xamarin.android/xbuild-frameworks/MonoAndroid/v1.0/mscorlib.dll
	$prefix/lib/xamarin.android/xbuild/Xamarin/Android/generator.exe
	$prefix/lib/xamarin.android/xbuild/Xamarin/Android/Xamarin.Android.Common.targets
	$prefix/lib/xamarin.android/xbuild/Xamarin/Android/Darwin/cross-arm

Other notes:

  * The `bundle-*.zip` filename has been has been changed to include a
    *hash* of the contents of various files, in particular
    `build-tools\mono-runtimes.*`. This was instigated via a
    conversation with @kumpera about making the filename more
    "idiot-proof": `mono-runtimes.props` contains *compiler flags*,
    and if any of those change, then *logically* the bundle should
    differ as well, as the mono runtimes may differ in significant
    ways. In theory, the `-v18` version should be used to track this,
    but this is a manual change, easy to overlook. The new `-hHASH`
    part of the filename should be more automagic.

    The new `<HashFileContents/>` task in `xa-prep-tasks.dll` is
    responsible for creating the hash value.

  * `Configuration.Java.Interop.Override.props` was moved into
    `build-tools/scripts`, becauase that would cleanup the root
    directory a bit.

  * OS-specific binaries are now placed into
    `$prefix/lib/xamarin.android/xbuild/Xamarin/Android/$(HostOS)`.
    On a theoretical plus side, this means that the same build
    directory can contain OS-specific binaries for multiple operating
    systems. (I don't know if anyone shares a build tree between e.g.
    macOS and Linux, but if anyone *does*...)

  * `$(MonoAndroidToolsDirectory)` should be considered *dead*, along
    with `$(MonoAndroidBinDirectory)`, as these should now *always*
    be the same directory as where `Xamarin.Android.Build.Tasks.dll`
    is located, or a `$(HostOS)` sub-directory.

  * `Xamarin.ProjectTools.csproj` needed to be updated to ensure that
    the build order was correct.

  * Remove all `[Obsolete]` and unreferenced members from
    `Xamarin.Android.Build.Utilities.dll`. There's too much in there,
    and it makes my head hurt trying to understand the
    interrelationships between it all. If it's not used, it's gone.

  * Work around an `xbuild`-ism: `%(_LlvmRuntime.InstallPath)` and
    `%(_MonoCrossRuntime.InstallPath)` *cannot* use MSBuild
    properties, e.g. `<InstallPath>$(HostOS)/</InstallPath>` doesn't
    work as desired; it's instead treated literally.
    Special-case `%(InstallPath)` until we fully migrate to MSBuild.

  * The changes to `src/monodroid/jni/Android.mk` are...weird. The
    removal of `-I$(topdir)/libmonodroid/zip`/etc. is to reduce use of
    non-existent paths, as `$(topdir)` isn't defined, so that's
    *actually* `-I/libmonodroid/zip`, which is nonsensical. So far,
    so good. What's *odd* is the addition of `$(LOCAL_PATH)` to
    `$(LOCAL_C_INCLUDES)`: This is needed so that
    `external/mono/support/zlib-helper.c` exports `CreateZStream` and
    related symbols, otherwise we get a unit test failure in
    `GzipStreamTest.Compression` due to an
    `EntryPointNotFoundException`, because `CreateZStream` isn't
    exported/public.

    What's odd here is that I don't understand what caused this
    behavior to change. Previous builds exported `CreateZStream`,
    otherwise the tests would fail, and I don't understand how any of
    the other changes in this PR would be at fault, though that's
    certainly the most plausible explanation.

    Regardless, `-Ijni` *first* (due to adding `$(LOCAL_PATH)` to
    `$(LOCAL_C_INCLUDES)`) is the desired behavior, so that
    `jni/config.h` is included, thus ensuring that `MONO_API` has the
    required definition when building `zlib-helper.c`.
@jonpryor jonpryor force-pushed the jonp-bin-structure branch from 1de3e3b to 688bb35 Compare July 28, 2017 02:01
@jonpryor jonpryor added the full-mono-integration-build For PRs; run a full build (~6-10h for mono bumps), not the faster PR subset (~2h for mono bumps) label Jul 28, 2017
@jonpryor jonpryor merged commit 97f08f7 into dotnet:master Jul 28, 2017
@jonpryor jonpryor mentioned this pull request Jul 28, 2017
jonpryor pushed a commit that referenced this pull request Sep 9, 2020
Fixes: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1193891

Changes: dotnet/java-interop@afbc5b3...ac914ce

  * dotnet/java-interop@ac914ce3: [ci] Add DevDiv required Roslyn analyzers, fix errors (#704)
  * dotnet/java-interop@a98c1ae1: [build] fix env vars causing gradle build error (#705)
  * dotnet/java-interop@75354b9f: [Java.Interop] Dispose, *then* Remove (#708)
  * dotnet/java-interop@8ea0bb28: [jnimarshalmethod-gen] Localizable errors (#696)

Commit dotnet/java-interop@ac914ce3 moved the location of
`NullableAttributes.cs`; update
`Xamarin.Android.Tools.JavadocImporter.csproj` appropriately.
@github-actions github-actions bot locked and limited conversation to collaborators Feb 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
do-not-merge PR should not be merged. full-mono-integration-build For PRs; run a full build (~6-10h for mono bumps), not the faster PR subset (~2h for mono bumps)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants