search

kiwix / kiwix-build

Kiwix & openZIM build engine
GNU General Public License v3.0
91 stars 43 forks source link

Apple Platforms updates #650

Closed rgaudin closed 11 months ago

rgaudin commented 11 months ago

This is a redo of #638 that is supposed to be more explicit. It includes:

Note that I did not rename the targets (contrary to #638). I think it may make sense to but this should be discussed first. It's important kiwix-build maintainers understand clearly the role of each target.

kelson42 commented 11 months ago

Small comment: we could probably use a more recent version of the Python action and Python version for this workflow.

rgaudin commented 11 months ago

Small comment: we could probably use a more recent version of the Python action and Python version for this workflow.

Let's do it on a separate PR that updates all actions on all workflows of the repo?

mgautierfr commented 11 months ago

The CI on libkiwix fails because this PR has published dependencies archive and its conflict with the configuration there. (https://github.com/kiwix/libkiwix/actions/runs/6796265893/job/18475983898)

I have made the PR #651 to avoid such overwriting project's deps. But it raise that we probably have things to change on projects' CI before merging this PR.

kelson42 commented 11 months ago

@mgautierfr #651 has been merged and CI is green here. Therefore my question: what are exactly the blockers to this PR?

mgautierfr commented 11 months ago

But it raise that we probably have things to change on projects' CI before merging this PR.

kelson42 commented 11 months ago

@mgautierfr Your comment is very vague, we have no clear ticket, I requested an "exact" answer. I don't get why this PR can not be merged and what has to be done now. All of this is urgent and last move seems to have been done 8 hours ago. I'm concern to have urgent tickets "lost in translation" without clear way forward.

rgaudin commented 11 months ago

Rebased ; deps properly sent to https://tmp.kiwix.org/ci/dev_preview/apple-xcf/ ; good to merge IMO @mgautierfr

mgautierfr commented 11 months ago

If we look at the job log in https://github.com/kiwix/libkiwix/actions/runs/6796265893/job/18475983898, we can see that tests are failing because the test binaries cannot link with libzim (which is compile for Mac OS X 13.0) :

dyld: Symbol not found: __ZNKSt3__115basic_stringbufIcNS_11char_traitsIcEENS_9allocatorIcEEE3strEv
  Referenced from: /Users/runner/./BUILD_native_dyn/INSTALL/lib/libzim.8.dylib (which was built for Mac OS X 13.0)
  Expected in: /usr/lib/libc++.1.dylib

As this PR has published the CI dependencies for libkiwix, and on libkiwix side we still compile libkiwix for macos 11, I suppose that we have a conflict here with the macos version for which binaries are compiled. But it is just a guess.

The PR #651 just make kiwix-build PRs not publishing CI dependencies to avoid not merged PR to break our CI. But if we merge this PR, it will goes on main branch and we will publish (as before) dependencies build for a macos version not expected on libkiwix (and other projects) side. So we have to update all our project's CI to build on macos 13 too.

I suspect it is the only change to do. But I can't totally be sure, so the vague comment "We have to do something on project side" (determine what is actually needed is left as an exercise for the PR creator 😉)

rgaudin commented 11 months ago

OK understood. There's an issue as we target macOS12 and not 13. Then I'll update libkiwix CI.

mgautierfr commented 11 months ago

There's an issue as we target macOS12 and not 13

Is this issue fixed ?

kelson42 commented 11 months ago

@rgaudin Concretly, this PR is blocked by which issue/PR?

rgaudin commented 11 months ago

Can we let go of all this pressure?

kelson42 commented 11 months ago

OK, but document clearly the dependencies. On this very specific topic we have a long history of tickets and PRs hanging around for weeks. This time is over, we can not afford it anymore unfortunately as we need to deliver.

rgaudin commented 11 months ago

Then read the discussion. @mgautierfr mentioned libkiwix not being able to link to a file apparently produced with this PR. This needs to be investigated and fixed if there's an issue. You requested an update on this after working hours and you're re-requesting one just after 8am. I don't work at night and don't intend to.

rgaudin commented 11 months ago

OK there's not much documentation on this but here are my findings:

So this all confirms that we can, indeed, build on macOS13, targeting macOS12, by setting the appropriate flag (as we do).

The problem with @mgautierfr's action is that dylib libzim is built using native_dyn target which doesn't uses the ApplePlatform described here.

I could fix the issue by creating a new macOSx64Mixed target and build libzim for it

class macOSx64Mixed(MixedMixin('macOS_x86_64'), ApplePlatformInfo):
    name = 'macOS_x86_64_mixed'
    arch = cpu = 'x86_64'
    host = 'x86_64-apple-darwin'
    target = 'x86_64-apple-macos'
    sdk_name = 'macosx'
    min_iphoneos_version = None
    min_macos_version = '12.0'
❯ kiwix-build --target-platform native_mixed libzim
❯ vtool -show-build ./BUILD_native_mixed/INSTALL/lib/libzim.8.dylib
./BUILD_native_mixed/INSTALL/lib/libzim.8.dylib:
Load command 10
      cmd LC_BUILD_VERSION
  cmdsize 32
 platform MACOS
    minos 14.0
      sdk 14.0
   ntools 1
     tool LD
  version 1015.7
❯ kiwix-build --target-platform macOS_x86_64_mixed libzim
❯ vtool -show-build ./BUILD_macOS_x86_64_mixed/INSTALL/lib/libzim.8.dylib
./BUILD_macOS_x86_64_mixed/INSTALL/lib/libzim.8.dylib:
Load command 10
      cmd LC_BUILD_VERSION
  cmdsize 32
 platform MACOS
    minos 12.0
      sdk 14.0
   ntools 1
     tool LD
  version 1015.7

@mgautierfr what should we do? Adding it would mean changing the CD targets for macOS

rgaudin commented 11 months ago

Unfortunately, libzim built this way lacks a lot of symbols (and weights about half the native_mixed version)

mgautierfr commented 11 months ago

Hum.. I'm not yet ready to drop native_platform. This is the base way to compile project and it should simply work. (Saying that, native build IS working, you can run the program on the platform you have build it. But not relevant here)

I see two options for now :

diff --git a/kiwixbuild/platforms/native.py b/kiwixbuild/platforms/native.py
index 8e10401..8b08da7 100644
--- a/kiwixbuild/platforms/native.py
+++ b/kiwixbuild/platforms/native.py
@@ -9,10 +9,12 @@ class NativePlatformInfo(PlatformInfo):

     def get_env(self):
         env = super().get_env()
         if neutralEnv('distname') == 'fedora':
             env['QT_SELECT'] = "5-64"
+        if neutralEnv('distname') == 'Darwin':
+            env['CFLAGS'] += ' '.join([env['CFLAGS'], '-mmacosx-version-min=12.0'])
         return env

 class NativeDyn(NativePlatformInfo):
     name = 'native_dyn'
diff --git a/kiwixbuild/dependencies/libzim.py b/kiwixbuild/dependencies/libzim.py
index 9a1fdca..b84e542 100644
--- a/kiwixbuild/dependencies/libzim.py
+++ b/kiwixbuild/dependencies/libzim.py
@@ -1,10 +1,10 @@
 from .base import (
     Dependency,
     GitClone,
     MesonBuilder)
-from kiwixbuild._global import option, get_target_step
+from kiwixbuild._global import option, get_target_step, neutralEnv

 class Libzim(Dependency):
     name = "libzim"
     force_build = True

@@ -46,5 +46,10 @@ class Libzim(Dependency):
         @property
         def library_type(self):
             if self.buildEnv.platformInfo.build == 'android':
                 return 'shared'
             return super().library_type
+
+
+        def set_env(self, env):
+            if neutralEnv('distname') == 'Darwin' or self.buildEnv.platformInfo.build == "iOS":
+                env['CFLAGS'] += ' '.join([env['CFLAGS'], '-mmacosx-version-min=12.0'])
rgaudin commented 11 months ago

With your approach we are setting all those options twice…

Another possibility is to change the minos tag after build using vtool.

mgautierfr commented 11 months ago

With your approach we are setting all those options twice…

My idea is to use only one of the two options. Why we are setting twice ? The first solution is only for native_build, so we don't set the option for other --targetPlatform. Or you are saying that we set option in two different places (in code) ?

mgautierfr commented 11 months ago

Another possibility is to change the minos tag after build using vtool.

Is minos a simple tag or it changes how it is compiled ?

rgaudin commented 11 months ago

Yes, We are defining it in (poorly named) ios.py and in native.py. This will create discrepancy and lead to maintenance issues

rgaudin commented 11 months ago

Another possibility is to change the minos tag after build using vtool.

Is minos a simple tag or it changes how it is compiled ?

Can be changed after compilation

❯ vtool -show-build ./BUILD_native_mixed/INSTALL/lib/libzim.8.dylib
./BUILD_native_mixed/INSTALL/lib/libzim.8.dylib:
Load command 10
      cmd LC_BUILD_VERSION
  cmdsize 32
 platform MACOS
    minos 14.0
      sdk 14.0
   ntools 1
     tool LD
  version 1015.7

❯ vtool -set-version-min macos 12.0 14.0 -replace -output ./BUILD_native_mixed/INSTALL/lib/libzim.8.dylib ./BUILD_native_mixed/INSTALL/lib/libzim.8.dylib

❯ vtool -show-build ./BUILD_native_mixed/INSTALL/lib/libzim.8.dylib
./BUILD_native_mixed/INSTALL/lib/libzim.8.dylib:
Load command 15
      cmd LC_VERSION_MIN_MACOSX
  cmdsize 16
  version 12.0
      sdk 14.0
rgaudin commented 11 months ago

But as with your solution, it only takes care of the min-macos-version and also duplicates definition

rgaudin commented 11 months ago

Could we tell NativeMixed to blend with corresponding platform in ios if on said platform?

mgautierfr commented 11 months ago

And with https://github.com/kiwix/kiwix-build/commit/b9c9a7ec90a5290af7617835516641cdf851de68 ? At least we set the version value in only one place.

Unfortunately, libzim built this way lacks a lot of symbols (and weights about half the native_mixed version)

Does it mean that your option with a macOSx64Mixed platform is not working ? It is strange. How it is for macOSArm64Mixed ?

rgaudin commented 11 months ago

It's not much different ; we still have macOS related stuff in ios and this in native… but we need to move on an this can be discussed in a separate ticket and fixed later. I am cherry-picking this commit (and fixing it!)

mgautierfr commented 11 months ago

Could we tell NativeMixed to blend with corresponding platform in ios if on said platform?

We could maybe with this kind of patch (not tested)

diff --git a/kiwixbuild/platforms/native.py b/kiwixbuild/platforms/native.py
index 8e10401..e8fc15a 100644
--- a/kiwixbuild/platforms/native.py
+++ b/kiwixbuild/platforms/native.py
@@ -1,20 +1,23 @@
 from .base import PlatformInfo, MixedMixin

+import platform
 from kiwixbuild.utils import pj
 from kiwixbuild._global import option, neutralEnv

-
-class NativePlatformInfo(PlatformInfo):
-    build = 'native'
-
-    def get_env(self):
-        env = super().get_env()
-        if neutralEnv('distname') == 'fedora':
-            env['QT_SELECT'] = "5-64"
-        return env
-
+if platform.system() == 'Darwin':
+    from .ios import macOSx64Mixed # Which platform ? It will depend of the actual architecture x86 or arm64
+    NativePlatformInfo = macOSx64Mixed
+else:
+    class NativePlatformInfo(PlatformInfo):
+        build = 'native'
+
+        def get_env(self):
+            env = super().get_env()
+            if neutralEnv('distname') == 'fedora':
+                env['QT_SELECT'] = "5-64"
+            return env

 class NativeDyn(NativePlatformInfo):
     name = 'native_dyn'
     static = False
     compatible_hosts = ['fedora', 'debian', 'Darwin']

But it seems a bit ugly for me.

I have tried few times ago to introduce the concept of backend (ie on what we are currently running). Maybe we will need to move on this.

rgaudin commented 11 months ago

Sorry it took so long, I wanted to make sure output is usable and realized that missing symbols were due to recent commits on libzim main (which breaks pylibzim ; will need to be updated)

rgaudin commented 11 months ago

@mgautierfr it's good to merge IMO ; please review