[Xamarin.Android.Build.Tasks] Fix libzip build on Linux

[akihikodaki]

• Remove an OS check preventing Linux from building the library
• Fix OutputLibraryPath for Linux

[monojenkins]

Hello! I'm the build bot for the Mono project.

I need approval from a Mono team member to build this pull request. A team member should reply with "approve" to approve a build of this pull request, "whitelist" to whitelist this and all future pull requests from this contributor, or "build" to explicitly request a build, even if one has already been done.

Contributors can ignore this message.

[dnfclas]

Hi @akihikodaki, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

[jonpryor]

I believe that the intention on Linux is to use/require the distro-provided libzip.so, in which case the current behavior to not build it is the correct behavior.

@grendello?

@akihikodaki: Is there a reason you want to build libzip.so instead of relying on the distro-provided version?

[akihikodaki]

The old OS check in BuildDependsOnLocal is different from #168, and it just breaks the build since it still tries to copy libzip.so.

@akihikodaki: Is there a reason you want to build libzip.so instead of relying on the distro-provided version?

No, I even wonder why it does have such a thing. It may be for a specific distribution (old Ubuntu?), but any guess on dependencies provided by distribution just doesn't make sense. If you still need to provide missing dependencies on a particular distribution, why don't you make a private repository (PPA for Ubuntu)?

[jonpryor]

I even wonder why it does have such a thing.

I'm confused about what "it" refers to in this sentence. Is it the building of libzip at all?

We need to build libzip for macOS and Windows, neither of which include libzip.

[akihikodaki]

@jonpryor I see. Not sure about macOS, but it may make sense for other OSes. Please don't mind.

[atsushieno]

We have explicit package setup for Ubuntu for now, implying that we depend on distro-based libzip.
https://github.com/xamarin/xamarin-android/blob/master/Makefile#L62

There is no reason to provide our own libzip build. Either distros or linux users should be responsible to offer a working one.

[grendello]

@akihikodaki The build system is prepared to provide dependencies for any Linux distro, see the link provided by @atsushieno. What does not make sense is to build the libraries/code that's readily available from the Linux distro's repository. We have support for Ubuntu because that's what we use, but we welcome contributions to support any other Linux distro.
And as for your suspicion that libzip is for "old Ubuntu", here are links to the packages from the latest Ubuntu versions:

http://packages.ubuntu.com/xenial/libzip4
http://packages.ubuntu.com/wily/libzip4

Debian:

https://packages.debian.org/jessie/libzip2

[akihikodaki]

Still _LinuxBuildLibZip property is available. Are you going to fix or remove it?

[atsushieno]

We should not accept build-tools/libzip/libzip.mdproj as that just results in extraneous build.

The other change is okay IMO to have (in case we would like to change mind and bundle libzip, depending on the future situation) but so far it's totally optional.

[akihikodaki]

We should not accept build-tools/libzip/libzip.mdproj as that just results in extraneous build.

Actually it builds nothing if '@(_LibZipTarget)' == '' ((host is not Darwin) AND ((host is not Linux) OR '$(_LinuxBuildLibZip)' == '')) because of Condition attributes of targets.

[grendello]

_LinuxBuildLibZip is not broken and there's no need to remove it. It works as designed - currently it prevents building (or rather bundling) of libzip on Ubuntu.

[akihikodaki]

_LinuxBuildLibZip intentionally forces to build it on other distributions and fails.

[grendello]

@akihikodaki I'm not sure what you mean by "intentionally forces to build"? We don't use "other distributions" - that's where there's room for community contributions. Your distro build is broken? Extend the detection and configuration process to support it, open a PR and we'll review it.

[akihikodaki]

@grendello Let me explain _LinuxBuildLibZip

 <_LinuxBuildLibZip Condition=" '$(_LinuxBuildLibZip)' == '' AND '$(HostOS)' == 'Linux' AND '$(HostOsName)' != 'Ubuntu' ">true</_LinuxBuildLibZip>

As you can see, if the host is Linux and the distribution is not Ubuntu, it tries to build libzip, but it always fails because of the bug this pull request fixes.

(Sorry I commented with a different identity by mistake)

[jonpryor]

As you can see, if the host is Linux and the distribution is not Ubuntu, it tries to build libzip, but it always fails because of the bug this pull request fixes.

As stated in prior comments, the intention is to use the system libzip on Linux. Your patch is counter to that intention, as it causes libzip to be built on every Linux distro.

I see two plausible solutions:

1. Fix the logic so that libzip is built on Linux distro's other than e.g. Ubuntu (and other "properly supported" distros).
2. "Properly support" your distro. This would include, at minimum, updating Makefile and adding linux-prepare-$(LINUX_DISTRO) and linux-prepare-$(LINUX_DISTRO)-$(LINUX_DISTRO_RELEASE) targets, which would ensure that libzip is installed on your distro, and update libzip.mdproj so that it doesn't build libzip.so on your particular distro.

[akihikodaki]

@jonpryor

As stated in prior comments, the intention is to use the system libzip on Linux. Your patch is counter to that intention, as it causes libzip to be built on every Linux distro.

Atcutally it's not to counter to the, as also stated:

Actually it builds nothing if '@(_LibZipTarget)' == '' ((host is not Darwin) AND ((host is not Linux) OR '$(_LinuxBuildLibZip)' == '')) because of Condition attributes of targets.

Doesn't the logic make sense?

[grendello]

@akihikodaki This PR doesn't fix the problem you indicate. If you could, please, re-read the comments (and especially the last comment by @jonpryor above which nicely and accurately outlines what needs to be done) and make the changes in the correct places to support your distribution, then we'd have a working solution instead of a discussion that appears to linger and lead nowhere. Locations of code to modify:

1. https://github.com/xamarin/xamarin-android/blob/master/build-tools/libzip/libzip.props#L4
2. https://github.com/xamarin/xamarin-android/blob/master/Makefile#L48-L124

Tip: run make prepare and then look at the generated Configuration.OperatingSystem.props file to see what OS detection capabilities you have for #1 above (code which generates the props file is right here)

[akihikodaki]

I'm quite a confident that I understand what you mean, but please let me explain my understanding to make sure that.

This pull request includes the following change:

-    <BuildDependsOnLocal Condition="'$(HostOS)' == 'Windows' OR '$(HostOS)' == 'Darwin'">
+    <BuildDependsOnLocal>

You suspect this change "causes libzip to be built on every Linux distro." If it should be built on other distributions, I should modify but should not remove BuildDependsOnLocal according to what you say. If I want to affirm builds on other distributions, I should also modify Makefile.

That's your opinion. However, I do NOT think it's correct. I explain why and would like you to correct if I am wrong.
Consider this pull request is applied and you are building it on Ubuntu.

LinuxBuildLibZip is 'true' only if it's being built on Linux distributions other than Ubuntu. In this case, $(LinuxBuildLibZip) is ''.

 <_LinuxBuildLibZip Condition=" '$(_LinuxBuildLibZip)' == '' AND '$(HostOS)' == 'Linux' AND '$(HostOsName)' != 'Ubuntu' ">true</_LinuxBuildLibZip>

_LibZipTarget will be defined only if '$(_LinuxBuildLibZip)' == 'true', so @(_LibZipTarget) will also be ''.

 <_LibZipTarget Include="host-Linux" Condition="$(AndroidSupportedHostJitAbisForConditionalChecks.Contains (':Linux:')) AND '$(_LinuxBuildLibZip)' == 'true' ">
(snip)
</_LibZipTarget>

As you say, BuildDependsOnLocal is defined even on Ubuntu if this pull request is applied, so the three targets (ResolveReferences, _Configure, and _Make) will be referred.

    <BuildDependsOnLocal>
      ResolveReferences;
      _Configure;
      _Make
    </BuildDependsOnLocal>

_Configure and _Make will output intermediate files and built binaries, so let's have a look at them.

  <Target Name="_Configure"
      Condition=" '@(_LibZipTarget)' != '' "
      DependsOnTargets="_SetCMakeListsTxtTimeToLastCommitTimestamp"
      Inputs="$(LibZipSourceFullPath)\CMakeLists.txt"
Outputs="$(IntermediateOutputPath)\%(_LibZipTarget.Identity)\Makefile">
    (snip)
  </Target>

" '@(_LibZipTarget)' != '' " is FALSE in this case, so _Configure won't run.

  <Target Name="_Make"
      Condition=" '@(_LibZipTarget)' != '' "
      Inputs="$(IntermediateOutputPath)\%(_LibZipTarget.Identity)\Makefile"
Outputs="@(Content)">
    (snip)
  </Target>

The Condition of _Make is also FALSE, so _Make won't run.

As the result, NOTHING will be built.

[borgdylan]

The last time I built XA, libzip was not automatically buillt yet was expected to be inside a particular directory. The build system did not copy the system's libzip to the correct place, if it copied it at all.

[akihikodaki]

83e3185: rebased to 3c55f93 and changed the commit message accordingly. The old commit message was confusing for the latest code base.

[akihikodaki]

Tested on Ubuntu 16.04 Xenial Xerus and it didn't build libzip as expected. (For curious people, I tested on WSL of build 14936, and the entire build failed because linker got aborted.)