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

Xamarin.Android build preparation utility #3051

Merged
merged 3 commits into from
Jun 7, 2019

Conversation

grendello
Copy link
Contributor

@grendello grendello commented Apr 30, 2019

Testing:

  • Default mode: run make prepare
  • Verbose debug: run make V=1 prepare
  • CI mode: run make PREPARE_CI=1 prepare
  • Help: run make prepare-help
  • Logs: bin/BuildDebug

Supported operating systems:

  • macOS
  • Linux

Windows support is planned, present but not fully implemented or tested.

This commit implements a make prepare replacement that does not rely on shell
scripts or MSBuild. The idea is that a standalone C# program can perform all the
steps in a more streamlined, more clear way than the current solution. One of
the main goals is to make the process more approachable by external
contributors, but also by the core developers who may want to change some
aspects of XA build preparation (e.g. Android platforms, Mono version etc) but
are not familiar with the build system. Towards that goal, the codebase is
designed so that the minimum amount of searching around it is necessary to
figure out where to make the desired change. Main location serving this purpose
is the build-tools/xaprepare/xaprepare/ConfigAndData directory. It should
be the only location where one needs to look in order to change all and any
aspects of XA preparation.

[TBC]

@grendello grendello added the do-not-merge PR should not be merged. label Apr 30, 2019
@grendello grendello force-pushed the xa/bootstrap branch 5 times, most recently from e8ae026 to bc91e42 Compare May 6, 2019 16:59
CODEOWNERS Outdated
@@ -20,6 +20,7 @@
/build-tools/manifest-attribute-codegen @jonpryor
/build-tools/scripts/PrepareWindows.targets @jonathanpeppers
/build-tools/timing @jonathanpeppers @radekdoulik
/build-tools/xabootstrap @grendello
Copy link
Member

Choose a reason for hiding this comment

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

Minor nitpick: should this be bootstrap or prepare? It's not like we're going to have multiple bootstrap systems in here...

Makefile Outdated
msbuild $(BOOTSTRAP_MSBUILD_ARGS) $(BOOTSTRAP_SOLUTION)

bootstrap:
$(MAKE) bootstrap-build
Copy link
Member

Choose a reason for hiding this comment

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

Why call $(MAKE) instead of depending on the bootstrap-build target?


1. bazelbuild/bazel (https://github.com/bazelbuild/bazel/)
2. google/desugar (https://android.googlesource.com/platform/external/desugar/+/master/)
3. mono/boringssl (https://github.com/mono/boringssl)
Copy link
Member

Choose a reason for hiding this comment

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

These changes imply that we're doing the wrong thing for the repo ThirdPartyNotices.txt file. The purpose of the repo-level ThirdPartyNotices.txt are for third party dependencies copied into this repo, not from submodules or NuGet packages or the like.

boringssl is included via submodule. linker is included via submodule. etc. These do not need to be present within this ThirdPartyNotices.txt file.

Third party code such as this does need to be listed in the generated and included-within-the-installer bin/$(Configuration)/lib/xamarin.android/ThirdPartyNotices.txt file.

@jonpryor
Copy link
Member

jonpryor commented May 6, 2019

Why is external/opentk modified?

@@ -0,0 +1,2 @@
http://www.kajabity.com
Copy link
Member

Choose a reason for hiding this comment

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

What's this file for?

Copy link
Member

Choose a reason for hiding this comment

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

There should be a README.md in this directory with required documentation...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Accidentally added, probably coming from the nuget

@@ -0,0 +1,12 @@
# Files to remove
Copy link
Member

Choose a reason for hiding this comment

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

This feels like material for README.md. Alternatively, more details need to be here.

For example, what constitutes "the migration"? How will this interact with checking out different branches?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just notes to myself, so that I don't forget to remove stuff later on (there's a LOT of it around).

{
partial class Windows : Dependencies
{
};
Copy link
Member

Choose a reason for hiding this comment

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

This isn't C++; trailing ; isn't needed. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

muscle memory :)

Dependencies.AddRange (programs);
Log.Todo ("special treatment for Mono - version checks and such");
}
};
Copy link
Member

Choose a reason for hiding this comment

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

This isn't C++; trailing ; isn't needed. :-)

new AndroidPlatformComponent ("platform-28_r04", "28"),

// arguments are: string name, string destDir, Uri relativeUrl = null, bool isMultiVersion = false, bool noSubdirectory = false
{ "docs-24_r01", "docs"},
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 this would be easier to read & understand if the extension method weren't used here. I'd never previously considered adding an List<SpecificType>.Add(args...) extension method before, and had to pause for a bit to understand what was going on.


public static readonly List<AndroidPlatform> AllPlatforms = new List<AndroidPlatform> {
// API, ID, Framework, Stable
{ 0, "0", null, false },
Copy link
Member

Choose a reason for hiding this comment

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

Likewise here: I'd never considered the ability to add a List<AndroidPlatform>.Add(...) extension method before. While "interesting", I'm not sure it's clearer than the alternative of new AndroidPlatform(...).

Additionally, while the extension method does add argument validation, ensuring that e.g. only one API-1 is added, that means each Add() call is O(n) instead of (amortized) O(1). This shouldn't matter with a list this small, but it is...odd.

{
static readonly Uri url = new Uri ("http://not.an/item");

public override string LicenseText => "not a license item";
Copy link
Member

Choose a reason for hiding this comment

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

What happened with the indentation here?

namespace Xamarin.Android.Bootstrap
{
[AttributeUsage (AttributeTargets.Class)]
class TPNAttribute : Attribute
Copy link
Member

Choose a reason for hiding this comment

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

Tabs, not spaces, please. :-)

{ "docs-24_r01", "docs"},
{ "android_m2repository_r16", Path.Combine ("extras", "android", "m2repository")},
{ "x86-28_r04", Path.Combine ("system-images", "android-28", "default", "x86"), new Uri ("sys-img/android/", UriKind.Relative)},
{$"android-ndk-r{AndroidNdkVersion}-{osTag}-x86_64", AndroidNdkDirectory},
Copy link
Member

Choose a reason for hiding this comment

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

Apparently AndroidNdkVersion isn't set when expected/required, because when the build attempts to download this url, it fails:

error : Unable to download URL `https://dl.google.com/android/repository/android-ndk-r-darwin-x86_64.zip` to `/Users/builder/android-archives/android-ndk-r-darwin-x86_64.zip`: Response status code does not indicate success: 404

Copy link
Member

Choose a reason for hiding this comment

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

BuildAndroidPlatforms.AndroidNdkVersion is const; how could this not be set/available here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The build fails because it uses make prepare and not the new bootstrapper which changed the way NDK references work. At this moment the build failure is expected, until I make the new bootstrapper the "default" in this PR

@jonpryor
Copy link
Member

jonpryor commented May 6, 2019

Also needed is some form of documentation, not just in build-tools/xabootstrap/README.md, but also in Documentation/building/unix/instructions.md.

There's a new make bootstrap target; what's it do? How does it differ from make prepare? It creates log files; where are those log files located, what can you expect to find there?

@jonpryor
Copy link
Member

jonpryor commented May 6, 2019

Given that e.g. bin/BuildDebug already contains log files, in the form of msbuild-*.binlog, should bin/BuildDebug/logs be renamed as bin/BuildDebug/bootstrap-logs? Or remove the logs directory and put them into bin/BuildDebug directly?

@grendello grendello force-pushed the xa/bootstrap branch 5 times, most recently from 6a3d4e3 to d518c8c Compare June 4, 2019 20:09
@grendello grendello force-pushed the xa/bootstrap branch 14 times, most recently from 139e1b8 to 25fabe6 Compare June 6, 2019 19:58
grendello and others added 3 commits June 7, 2019 08:38
Testing:

 * Default mode: run `make prepare`
 * Verbose debug: run `make V=1 prepare`
 * CI mode: run `make PREPARE_CI=1 prepare`
 * Help: run `make prepare-help`
 * Logs: `bin/BuildDebug`

Supported operating systems:

 * macOS
 * Linux

Windows support is planned, present but not fully implemented or tested.

This commit implements a `make prepare` replacement that does not rely on shell
scripts or MSBuild. The idea is that a standalone C# program can perform all the
steps in a more streamlined, more clear way than the current solution. One of
the main goals is to make the process more approachable by external
contributors, but also by the core developers who may want to change some
aspects of XA build preparation (e.g. Android platforms, Mono version etc) but
are not familiar with the build system. Towards that goal, the codebase is
designed so that the minimum amount of searching around it is necessary to
figure out where to make the desired change. Main location serving this purpose
is the `build-tools/xaprepare/xaprepare/ConfigAndData` directory. It should
be the *only* location where one needs to look in order to change all and any
aspects of XA preparation.

[TBC]
Usage: `xaprepare.exe -s=AndroidToolchain`

The step will *only* install Android SDK+NDK as well as the OpenJDK distribution
we provision. No other programs are installed or updated.
Allows a non-authorized https-based clone for local use cases, and cleans
up various logging events to ensure we don't leak secrets. Also includes
some minor azure-pipelines.yaml cleanup.
@jonpryor jonpryor merged commit 9302d51 into dotnet:master Jun 7, 2019
@grendello grendello deleted the xa/bootstrap branch June 7, 2019 17:59
@github-actions github-actions bot locked and limited conversation to collaborators Jan 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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.

6 participants