Skip to content

Conversation

luhenry
Copy link
Contributor

@luhenry luhenry commented Oct 4, 2018

No description provided.

@luhenry luhenry requested a review from jonpryor as a code owner October 4, 2018 17:59
@luhenry luhenry force-pushed the mono-2018-06 branch 2 times, most recently from 421e6b0 to 50026eb Compare October 5, 2018 17:33
@luhenry luhenry added the do-not-merge PR should not be merged. label Oct 5, 2018
@luhenry
Copy link
Contributor Author

luhenry commented Oct 5, 2018

Depends on mono/mono#10936


namespace Xamarin.Android.BuildTools.PrepTasks
{
public sealed class GitAbbreviatedCommitHash : Git
Copy link
Contributor

Choose a reason for hiding this comment

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

Why introduce this new task instead of using the existing <GitCommitHash/>?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it would be cleaner and simpler, but that's up to you. Should I add a CommitHash output parameter to the pre-existing GitCommitHash.cs file and use that instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please do, otherwise we'll have two tasks running around which are semantically very similar, which will confuse me when doing new additions ("should we use <GitCommitHash/> or <GitAbbreviatedCommitHash/>?"), especially when <GitCommitHash/> already had an AbbreviatedCommitHash output property.

@luhenry luhenry force-pushed the mono-2018-06 branch 2 times, most recently from 3af6695 to b303067 Compare October 8, 2018 18:17
@luhenry luhenry changed the base branch from mono-2018-06 to master October 9, 2018 15:02
@luhenry luhenry changed the title [mono-2018-06] [mono-sdks] Package BCL from dedicated SDKs target. [mono-sdks] Package BCL from dedicated SDKs target. Oct 9, 2018
@luhenry luhenry force-pushed the mono-2018-06 branch 2 times, most recently from b1c29f6 to 4781b7b Compare October 9, 2018 15:08
@luhenry luhenry changed the title [mono-sdks] Package BCL from dedicated SDKs target. [mono-sdks] Try to download Mono runtimes and BCL before building them. Oct 9, 2018
<Exec
Command="make DISABLE_IOS=1 $(MakeConcurrency) @(_MonoRuntime->'package-android-%(Identity)', ' ') @(_MonoCrossRuntime->'package-android-%(Identity)', ' ') $(_MonoSdksParameters)"
Condition=" !Exists('$(MonoSourceFullPath)\sdks\out\.stamp-android-$(_MonoSdksConfiguration)-$(_MonoGitCommitHash)-$(HostOS)-download') "
Command="curl --location --silent --show-error https://xamjenkinsartifact.blob.core.windows.net/mono-sdks/android-$(_MonoSdksConfiguration)-$(_MonoGitCommitHash)-$(HostOS).tar.gz | tar -xvf -"
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of <Exec/> to curl, I would prefer that you use the <DownloadUri/> task: https://github.com/xamarin/xamarin-android/blob/master/build-tools/xa-prep-tasks/Xamarin.Android.BuildTools.PrepTasks/DownloadUri.cs

It looks like a point to this PR is to have xamarin-android download the mono-produced bundle directly. Assuming that's true, then a major benefit to this is that it means that Windows should thus be able to build xamarin-android without a previous macOS xamarin-android build, which would be sweet. (It would instead require that a prebuilt mono-produced bundle exist.)

Using <DownloadUri/> would thus (plausibly) make it possible for Windows to execute this target file.

@jonathanpeppers: Additional thoughts on Windows support?

Copy link
Contributor

Choose a reason for hiding this comment

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

Additional thoughts:

  1. The mono bundle should be a .zip file, not .tgz, so that Windows can plausibly extract the file.
  2. curl | tar xzf - isn't very resilient to network failures, and could result in a "corrupt" local tree.

Another benefit to using <DownloadTask/> is that it ensures that the entire file is properly downloaded before execution continues.

Furthermore, if <DownloadTask/> is used, then the downloaded file should not be downloaded into $CWD. It should instead be downloaded into $(AndroidToolchainCacheDirectory) (usually $HOME/android-archives), so that if someone does git clean -xdff, the file doesn't need to be re-downloaded again.

See e.g. build-tools/android-toolchain/android-toolchain.targets: https://github.com/xamarin/xamarin-android/blob/608d950a703ddb527258565c2a0705aa37f71789/build-tools/android-toolchain/android-toolchain.targets#L55-L58

Copy link
Member

Choose a reason for hiding this comment

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

@luhenry ping me when you have this using DownloadUri and I will test this on Windows.

In theory, this would be great for our Windows builds, which currently go red whenever we change the mono build, or bump mono.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is not the whole mono-bundle that XA relies on, it only contains the mono bits. I'll try doing it with <DownloadUri/>.

@jonpryor
Copy link
Contributor

@luhenry: the bundle which mono is producing: Will that contain Windows binaries?

@luhenry
Copy link
Contributor Author

luhenry commented Oct 10, 2018

@jonpryor the bundle will contain all the runtimes, BCL and LLVM which are currently being produced by the SDKs for Android (ie also the host-mxe-Win32, host-mxe-Win64, cross-arm-win, cross-arm64-win, cross-x86-win and cross-x86_64-win).

@luhenry luhenry force-pushed the mono-2018-06 branch 2 times, most recently from 5476d88 to 4f5039d Compare October 11, 2018 17:38
@luhenry luhenry removed the do-not-merge PR should not be merged. label Oct 11, 2018
@jonpryor
Copy link
Contributor

The latest build failed:

xamarin-android/bin/Debug/lib/xamarin.android/xbuild/Xamarin/Android/Xamarin.Android.Common.targets(1883,2): error XA2002: Can not resolve reference: `xunit.execution.desktop`, referenced by `Xunit.NetCore.Extensions`. Please add a NuGet package or assembly reference for `xunit.execution.desktop`, or remove the reference to `Xunit.NetCore.Extensions`. 

@luhenry
Copy link
Contributor Author

luhenry commented Oct 12, 2018

@jonpryor that’s been fixed. All builds are green now.

</Target>

<Target Name="_Build"
Condition=" '@(_MonoRuntime)' != '' Or '@(_MonoCrossRuntime)' != '' Or '@(_MonoBcl)' != '' Or '@(_LlvmRuntime)' != '' "
Copy link
Contributor

Choose a reason for hiding this comment

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

This Condition is a tad odd: why is '@(_MonoBcl)' != '' in here? The @(_MonoBcl) declaration is not conditional, i.e. it's always defined, so unless you imagine updating things in the future so that @(_MonoBcl) is empty...why does @(_MonoBcl) need to be checked?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to match what's already there. If you want I can fix that in a follow-up PR, but I would prefer not to have to wait for a full re-run just for that.

Copy link
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

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

@luhenry I tested this PR on Windows, which lead to wonder if there is still an issue here.

I disabled this <Error /> to ignore the download of our Xamarin.Android mono bundle:
https://github.com/xamarin/xamarin-android/blob/320cb0f66730e7107cc17310b99cd540605a234d/build-tools/download-bundle/download-bundle.targets#L29-L32

Then /t:Prepare seemed to work on Windows just fine.

Then /t:Build failed with:

mono-runtimes.targets(176,5): error MSB3073: The command "make DISABLE_IOS=1 configure-mono" exited with code 9009. 

I see a _DownloadArchive target running getting a 404?

DownloadUri
    Parameters
        SourceUris = https://xamjenkinsartifact.blob.core.windows.net/mono-sdks/android-debug-226e91a0644d69387e822867fe7e8bec2f875b5f-Windows.tar.gz
        DestinationFiles = C:\Users\jopepper\android-archives\android-debug-226e91a0644d69387e822867fe7e8bec2f875b5f-Windows.tar.gz
     DownloadUri:
       SourceUris:
         https://xamjenkinsartifact.blob.core.windows.net/mono-sdks/android-debug-226e91a0644d69387e822867fe7e8bec2f875b5f-Windows.tar.gz
       DestinationFiles:
         C:\Users\jopepper\android-archives\android-debug-226e91a0644d69387e822867fe7e8bec2f875b5f-Windows.tar.gz
     Downloading `https://xamjenkinsartifact.blob.core.windows.net/mono-sdks/android-debug-226e91a0644d69387e822867fe7e8bec2f875b5f-Windows.tar.gz` to `C:\Users\jopepper\android-archives\.android-debug-226e91a0644d69387e822867fe7e8bec2f875b5f-Windows.tar.gz.download`.
    Warnings
        C:\Users\jopepper\Desktop\Git\xamarin-android\src\mono-runtimes\mono-runtimes.targets(291,5): Unable to download URL `https://xamjenkinsartifact.blob.core.windows.net/mono-sdks/android-debug-226e91a0644d69387e822867fe7e8bec2f875b5f-Windows.tar.gz` to `C:\Users\jopepper\android-archives\android-debug-226e91a0644d69387e822867fe7e8bec2f875b5f-Windows.tar.gz`: Response status code does not indicate success: 404 (The specified blob does not exist.). [C:\Users\jopepper\Desktop\Git\xamarin-android\src\mono-runtimes\mono-runtimes.csproj]
        C:\Users\jopepper\Desktop\Git\xamarin-android\src\mono-runtimes\mono-runtimes.targets(291,5): Response status code does not indicate success: 404 (The specified blob does not exist.). [C:\Users\jopepper\Desktop\Git\xamarin-android\src\mono-runtimes\mono-runtimes.csproj]
     The previous error was converted to a warning because the task was called with ContinueOnError=true.
     The previous error was converted to a warning because the task was called with ContinueOnError=true.
     Build continuing because "ContinueOnError" on the task "DownloadUri" is set to "True".

Obviously calling make won't work, but normally this target gets skipped if the download worked and was extracted?

I looked at the Jenkins log, and it seems to be doing similar on MacOS?

Unable to download URL `https://****.blob.core.windows.net/mono-sdks/android-debug-226e91a0644d69387e822867fe7e8bec2f875b5f-Darwin.tar.gz` to `/Users/builder/android-archives/android-debug-226e91a0644d69387e822867fe7e8bec2f875b5f-Darwin.tar.gz`: 404 (The specified blob does not exist.)
...
_Autogen:
  make DISABLE_IOS=1 configure-mono
  cd /Users/builder/jenkins/workspace/xamarin-android-pr-builder/xamarin-android/external/mono && PATH=:$PATH NOCONFIGURE=1 ./autogen.sh
  Running libtoolize...

@luhenry
Copy link
Contributor Author

luhenry commented Oct 12, 2018

As discussed on Slack, for the step not to get a 404, we first need to setup the Jenkins jobs on Mono's end which builds the archive for XA to consume. It hasn't been done yet, but will be done very soon (it's next on my TODO list!). The fallback to building is meant to be transparent, and it's actually a good thing that we verified it's working :)

Also, this "bundle" is in no way a replacement for XA's existing bundle, so it shouldn't be expected to work on Windows. And as @jonpryor already noted, it's using tar to compress which wouldn't work on Windows anyway, even if that'll also soon be moved to a zip archive as well.

IgnoreStandardErrorWarningFormat="True"
WorkingDirectory="$(MonoSourceFullPath)\sdks\builds"
WorkingDirectory="$(MonoSourceFullPath)\sdks\out"
/>
Copy link
Member

Choose a reason for hiding this comment

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

Yeah this command won't work on Windows when/if we get here. Is there a managed C# equivalent?

@jonathanpeppers
Copy link
Member

Yeah, testing on Windows was to see how close we could get to removing XA's existing bundle. I expected to hit libzip-related failures.

I'll try this again after the mono bundle is downloading and working, and we would need either the zip or a fix to extract the *.tar.gz file.

@jonpryor jonpryor merged commit f970cd5 into dotnet:master Oct 12, 2018
jonpryor added a commit that referenced this pull request Oct 12, 2018
…mono (#2265)"

This reverts commit f970cd5.

Unfortunately, it [broke the build][0]:

	  libtool:   error: Could not determine the host path corresponding to
	  libtool:   error: Could not determine the host path corresponding to
	  libtool:   error:   '/Users/builder/jenkins/workspace/xamarin-android/xamarin-android/external/mono/sdks/builds/android-host-mxe-Win64-debug/mono/mini/.libs'
	  libtool:   error:   '/Users/builder/jenkins/workspace/xamarin-android/xamarin-android/external/mono/sdks/builds/android-host-mxe-Win64-debug/mono/mini/.libs'
	  libtool:   error: Continuing, but uninstalled executables may not work.

Looks like the MXE+Windows build side was broken. :-(

Revert commit f970cd5 until we figure things out.

[0]: https://jenkins.mono-project.com/view/Xamarin.Android/job/xamarin-android/1207/
luhenry added a commit to luhenry/xamarin-android that referenced this pull request Oct 15, 2018
@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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants