[Android] Fixes wrong paths to so-files. Disables LIPO-routines for Android. #25682

vgorloff · 2019-06-22T12:18:24Z

LIPO routines cause build error when cmake tries to copy multiple files into one file. This doesn't make any sense. Also, fat binaries not needed on Android platform since SO-files placed into per-architecture folder inside apk-file.

[190/212] Generating ../../../lib/swift/android/libswiftReflection.a
FAILED: lib/swift/android/libswiftReflection.a
cd /swift-everywhere-toolchain/Build/swift/stdlib/public/Reflection && cmake -E copy /swift-everywhere-toolchain/Build/swift/lib/swift/android/armv7/libswiftReflection.a /swift-everywhere-toolchain/Build/swift/lib/swift/android/aarch64/libswiftReflection.a /swift-everywhere-toolchain/Build/swift/lib/swift/android/i686/libswiftReflection.a /swift-everywhere-toolchain/Build/swift/lib/swift/android/x86_64/libswiftReflection.a /swift-everywhere-toolchain/Build/swift/./lib/swift/android/libswiftReflection.a
Error: Target (for copy command) "/swift-everywhere-toolchain/Build/swift/./lib/swift/android/libswiftReflection.a" is not a directory.

Thats why this PR completely disables LIPO-routines for Android.

Here is a current implementation of function for making FAT-binary (cmake/modules/AddSwift.cmake)

  if(${LIPO_SDK} IN_LIST SWIFT_APPLE_PLATFORMS)
    if(LIPO_CODESIGN)
      set(codesign_command COMMAND "codesign" "-f" "-s" "-" "${LIPO_OUTPUT}")
    endif()
    # Use lipo to create the final binary.
    add_custom_command_target(unused_var
        COMMAND "${SWIFT_LIPO}" "-create" "-output" "${LIPO_OUTPUT}" ${source_binaries}
        ${codesign_command}
        CUSTOM_TARGET_NAME "${LIPO_TARGET}"
        OUTPUT "${LIPO_OUTPUT}"
        DEPENDS ${source_targets})
  else()
    # We don't know how to create fat binaries for other platforms.
    add_custom_command_target(unused_var
        COMMAND "${CMAKE_COMMAND}" "-E" "copy" "${source_binaries}" "${LIPO_OUTPUT}"
        CUSTOM_TARGET_NAME "${LIPO_TARGET}"
        OUTPUT "${LIPO_OUTPUT}" # <<< THIS IS A FILE in ANDROID build, NOT A FOLDER
        DEPENDS ${source_targets})
  endif()

Another issue about wrong path to so-files in generated cmake_install.cmake files. For example below the file swift/android/libswiftSwiftOnoneSupport.dylib used instead of swift/android/armv7/libswiftSwiftOnoneSupport.so

if("x${CMAKE_INSTALL_COMPONENT}x" STREQUAL "xstdlibx" OR NOT CMAKE_INSTALL_COMPONENT)
  file(INSTALL DESTINATION "${CMAKE_INSTALL_PREFIX}/lib/swift/android" TYPE FILE PERMISSIONS OWNER_READ OWNER_WRITE OWNER_EXECUTE GROUP_READ GROUP_EXECUTE WORLD_READ WORLD_EXECUTE FILES "/swift-everywhere-toolchain/Build/darwin/swift/./lib/swift/android/libswiftSwiftOnoneSupport.dylib")
endif()

…a install". Disables "lipo / fat binary" routines to avoid build error for Android builds.

vgorloff · 2019-06-22T12:19:54Z

@swift-ci Please test

compnerd · 2019-06-24T04:08:58Z

I'm not sure I understand this. The lipo path is already severed on any non-MachO target, all it does is a copy. The path is still present for uniformity.

vgorloff · 2019-06-24T07:35:11Z

In current implementation four files (/android/armv7/libswiftReflection.a, /android/aarch64/libswiftReflection.a, /android/i686/libswiftReflection.a, /android/x86_64/libswiftReflection.a) built inside per-architecture folders (armv7, aarch64, i686, x86_64) copied into one file (/android/libswiftReflection.a). This is illegal file system operation. System can't copy four files into one.

That's why I disabled LIPO routines for Android.

FAILED: lib/swift/android/libswiftReflection.a
cd /build/swift/stdlib/public/Reflection && cmake -E copy \
/build/swift/lib/swift/android/armv7/libswiftReflection.a \
/build/swift/lib/swift/android/aarch64/libswiftReflection.a \
/uild/swift/lib/swift/android/i686/libswiftReflection.a \
/build/swift/lib/swift/android/x86_64/libswiftReflection.a \
/build/swift/./lib/swift/android/libswiftReflection.a

Error: Target (for copy command)
"/build/swift/./lib/swift/android/libswiftReflection.a" is not a directory.

vgorloff · 2019-06-24T07:51:15Z

Here is some information: ABI Management on the Android Platform

Native code in app packages

Both the Play Store and Package Manager expect to find NDK-generated libraries on filepaths inside the APK matching the following pattern:
/lib/[abi]/lib<name>.so
In a fat APK, each library resides under a directory whose name matches a corresponding ABI. For example, a fat APK may contain:
/lib/armeabi/libfoo.so
/lib/armeabi-v7a/libfoo.so
/lib/arm64-v8a/libfoo.so
/lib/x86/libfoo.so
/lib/x86_64/libfoo.so

So, this PR eliminate unneeded LIPO routines since Android organises fat APKs in a way so that each library resides under a directory whose name matches a corresponding ABI.

compnerd · 2019-06-24T23:06:26Z

Ah, I see what you mean. The architecture variant support is currently broken and laying it out that way doesn’t really make things easier to rework. I would rather that you work to remove the copy operation in https://github.com/apple/swift/blob/master/cmake/modules/AddSwift.cmake#L635 instead.

vgorloff · 2019-06-25T06:06:31Z

Ah, I see what you mean. The architecture variant support is currently broken and laying it out that way doesn’t really make things easier to rework. I would rather that you work to remove the copy operation in https://github.com/apple/swift/blob/master/cmake/modules/AddSwift.cmake#L635 instead.

Ok. I will try to leave LIPO routines untouched. Instead will try to avoid copy operation.

vgorloff · 2019-06-25T10:43:31Z

I removed copy operation in LIPO routines. In two places we still need to check for (if sdk IN_LIST SWIFT_APPLE_PLATFORMS) to avoid CMake project consistency errors.

compnerd · 2019-06-25T21:10:03Z

CC: @drodriguez

drodriguez

Reading the diff is very difficult to figure out if something will work or not.

Many people have tried this, and we have fail for one or other reason. My last try is at #19432. There's a lot more pieces there. I will have to remember what each one was doing. One thing that I think might not work for free is that the Swift compiler only looks in lib/foo.so, and not in lib/{arch}/foo.so. That's why the copy still has to happen, and the install copied the “fat” file. In the mentioned PR there's also changes in the compiler to take that movement into account, but worst part is that other projects hardcode lib/, and need to be changed in a compatible way too (I did some of those PRs, and I think I remember one being accepted, but later being reverted).

I don’t think that old PR will apply cleanly anymore, but maybe you want to review it and incorporate ideas from it into this. Maybe applying it only to Android is a way, but if you want to catch all the little problems, I might suggest using Linux as the test bed. It might uncover a lot of things.

drodriguez · 2019-06-25T22:29:14Z

cmake/modules/AddSwift.cmake

-        COMMAND "${CMAKE_COMMAND}" "-E" "copy" "${source_binaries}" "${LIPO_OUTPUT}"
-        CUSTOM_TARGET_NAME "${LIPO_TARGET}"
-        OUTPUT "${LIPO_OUTPUT}"
-        DEPENDS ${source_targets})


Minor: One thing I did here is adding a precondition. If someone were to add a new call to _add_swift_lipo_target, CMake will refuse to generate for any SDK which is not Darwin.

drodriguez · 2019-06-25T22:33:00Z

cmake/modules/AddSwift.cmake

@@ -2006,7 +1999,7 @@ function(add_swift_target_library name)
        endif()
      endif()

-      if(NOT SWIFTLIB_OBJECT_LIBRARY)
+      if(NOT SWIFTLIB_OBJECT_LIBRARY AND sdk IN_LIST SWIFT_APPLE_PLATFORMS)
        # Add dependencies on the (not-yet-created) custom lipo target.


You obviously do not have to add dependencies to the lipo targets, but you need to add dependencies to the architecture specific targets. A full CMake build might even fail if some dependent tries to be built before a dependency, but partial builds will be specially affected.

drodriguez · 2019-06-25T22:46:30Z

cmake/modules/AddSwift.cmake

@@ -2128,6 +2123,17 @@ function(add_swift_target_library name)
                                   ARCHIVE DESTINATION "lib${LLVM_LIBDIR_SUFFIX}/${resource_dir}/${resource_dir_sdk_subdir}/${SWIFT_PRIMARY_VARIANT_ARCH}"
                                   COMPONENT "${SWIFTLIB_INSTALL_IN_COMPONENT}"
                                   PERMISSIONS ${file_permissions})
+      elseif(sdk STREQUAL ANDROID)
+        foreach(arch ${SWIFT_SDK_ANDROID_ARCHITECTURES})


@compnerd: do you remember why on the Windows branch you went with the primary variant architecture, instead of installing all of them? Below, around L2144 you do copy all architectures for the import libraries, for example.

There might be a way of doing all non Darwin SDKs in the same way (Linux might also benefit from multi-architectures). One can use ${sdk_supported_archs} or ${SWIFT_SDK_${sdk}_ARCHITECTURES} for the architecture list.

vgorloff · 2019-06-26T18:55:08Z

Thank you!

The PR #19432 contains changes which helps to solve problem in general. This PR addressed only small thing, only for concrete platform (Android). Due CMake configuration complexity (for Swift and CoreLibs) it might take me a lot of time to understand and apply changes from #19432.

What I can do (for this PR) is following:

Update this PR with mentioned corrections.
Setup Vagrant box and perform "multiple architecture build" on Linux.
Make sure that Linux build is not broken and Tests are passing.

drodriguez · 2019-06-26T19:10:19Z

I would be happy if you can confirm that the Swift compiler will look in the per-architecture directories for the linking phase. I know it does for the compiling phase, but some of the changes in #19432 were related to teach the compiler where to find the linker dependencies. If you have questions about the changes there, don’t hesitate to ask me.

PS: If you don’t have a Linux box at hand, do not worry about it. I mention it was the easier way because it is if you have the box already. In other case, it is more hassle, if you keep your changes gated to Android, it should not affect the Linux builds at all.

shahmishal · 2020-10-01T06:59:46Z

Please update the base branch to main by Oct 5th otherwise the pull request will be closed automatically.

How to change the base branch: (Link)
More detail about the branch update: (Link)

Fixes wrong paths to so-files which cause missed files error in "ninj…

5fbe121

…a install". Disables "lipo / fat binary" routines to avoid build error for Android builds.

vgorloff changed the title ~~Fixes wrong paths to so-files which cause missed files error in "ninj…~~ [Android] Fixes wrong paths to so-files. Disables LIPO-routines for Android. Jun 22, 2019

Changes requested in PR review.

a09a5eb

drodriguez reviewed Jun 25, 2019

View reviewed changes

shahmishal closed this Oct 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Android] Fixes wrong paths to so-files. Disables LIPO-routines for Android. #25682

[Android] Fixes wrong paths to so-files. Disables LIPO-routines for Android. #25682

vgorloff commented Jun 22, 2019

vgorloff commented Jun 22, 2019

compnerd commented Jun 24, 2019

vgorloff commented Jun 24, 2019 •

edited

Loading

vgorloff commented Jun 24, 2019

compnerd commented Jun 24, 2019

vgorloff commented Jun 25, 2019

vgorloff commented Jun 25, 2019 •

edited

Loading

compnerd commented Jun 25, 2019

drodriguez left a comment

drodriguez Jun 25, 2019

drodriguez Jun 25, 2019

drodriguez Jun 25, 2019 •

edited

Loading

vgorloff commented Jun 26, 2019

drodriguez commented Jun 26, 2019

shahmishal commented Oct 1, 2020

[Android] Fixes wrong paths to so-files. Disables LIPO-routines for Android. #25682

[Android] Fixes wrong paths to so-files. Disables LIPO-routines for Android. #25682

Conversation

vgorloff commented Jun 22, 2019

vgorloff commented Jun 22, 2019

compnerd commented Jun 24, 2019

vgorloff commented Jun 24, 2019 • edited Loading

vgorloff commented Jun 24, 2019

compnerd commented Jun 24, 2019

vgorloff commented Jun 25, 2019

vgorloff commented Jun 25, 2019 • edited Loading

compnerd commented Jun 25, 2019

drodriguez left a comment

Choose a reason for hiding this comment

drodriguez Jun 25, 2019

Choose a reason for hiding this comment

drodriguez Jun 25, 2019

Choose a reason for hiding this comment

drodriguez Jun 25, 2019 • edited Loading

Choose a reason for hiding this comment

vgorloff commented Jun 26, 2019

drodriguez commented Jun 26, 2019

shahmishal commented Oct 1, 2020

vgorloff commented Jun 24, 2019 •

edited

Loading

vgorloff commented Jun 25, 2019 •

edited

Loading

drodriguez Jun 25, 2019 •

edited

Loading