Skip to content

Android: add bootstrap build support and fix tests #2417

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

Merged
merged 1 commit into from
Dec 9, 2019

Conversation

finagolfin
Copy link
Member

@finagolfin finagolfin commented Nov 22, 2019

With this pull, I got SPM built natively on Android and all the tests passing with the Nov. 8 source snapshot from the master branch.

@finagolfin finagolfin requested a review from aciidgh as a code owner November 22, 2019 13:38
@@ -681,7 +685,7 @@ def process_runtime_libraries(build, args, lib_path):
cmd = [args.swiftc_path, "-emit-library", "-o", runtime_lib_path,
"-Xlinker", "--whole-archive",
"-Xlinker", input_lib_path,
"-Xlinker", "--no-whole-archive", "-lswiftGlibc",
"-Xlinker", "--no-whole-archive", "-lswiftGlibc", "-lFoundation",
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the only part of this pull that affects non-Android platforms. I had issues for a long time with getting the bootstrapped SPM to interpret its own manifest and dump it to JSON (as mentioned in #2396) and it turned out this was why.

The problem is that libPackageDescription.so depends on some functions from Foundation, but doesn't list it as a required dependency:

> readelf -sW 512-release/usr/lib/swift/pm/4_2/libPackageDescription.so | ag Foundation
   160: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND $s10Foundation11JSONEncoderCACycfc
   161: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND $s10Foundation11JSONEncoderCMa
   162: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND $sSS10FoundationE4data8encodingSSSgAA4DataVh_SSAAE8EncodingVtcfC
   163: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND $sSS10FoundationE8EncodingV4utf8ACvau
  1616: 000000000001d130    78 FUNC    LOCAL  HIDDEN    11 $s10Foundation4DataV15_RepresentationOWOe
  2341: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND $s10Foundation11JSONEncoderCACycfc
  2342: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND $s10Foundation11JSONEncoderCMa
  2445: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND $sSS10FoundationE4data8encodingSSSgAA4DataVh_SSAAE8EncodingVtcfC
  2446: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND $sSS10FoundationE8EncodingV4utf8ACvau
> readelf -d 512-release/usr/lib/swift/pm/4_2/libPackageDescription.so| ag library
 0x0000000000000001 (NEEDED)             Shared library: [libswiftSwiftOnoneSupport.so]
 0x0000000000000001 (NEEDED)             Shared library: [libswiftCore.so]
 0x0000000000000001 (NEEDED)             Shared library: [libswiftGlibc.so]
 0x0000000000000001 (NEEDED)             Shared library: [libstdc++.so.6]
 0x0000000000000001 (NEEDED)             Shared library: [libm.so.6]
 0x0000000000000001 (NEEDED)             Shared library: [libgcc_s.so.1]
 0x0000000000000001 (NEEDED)             Shared library: [libc.so.6]
 0x000000000000001d (RUNPATH)            Library runpath: [/home/buildnode/jenkins/workspace/oss-swift-5.1-package-linux-ubuntu-18_04/build/buildbot_linux/swift-linux-x86_64/lib/swift/linux:$ORIGIN/../../linux]

These results were from the official 5.1.2 release for linux x86_64, I get the same results with the latest snapshots from master.

The Android dynamic linker enforces that even if a executable or library links against libPackageDescription.so and lists libFoundation.so as a dependency there, it isn't transitively applied to libPackageDescription.so, unlike the GNU dynamic linker historically, though that may be changing. It appears that's why this hasn't hit on linux, as the Swift compiler seems to automatically add the needed Foundation dependency to a linux executable I tested linking against libPackageDescription.so, but that doesn't help on Android since it enforces that each library list its requirements itself.

@@ -270,6 +270,10 @@ class Target(object):
if args.llbuild_link_framework:
other_args.extend(["-F", args.llbuild_build_dir])

# Don't use GNU strerror_r on Android.
if 'ANDROID_DATA' in os.environ:
other_args.extend(["-Xcc", "-U_GNU_SOURCE"])
Copy link
Contributor

Choose a reason for hiding this comment

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

I am trying to move SwiftPM away from this bootstrap script and rely the CMake 3.15.1 which has builtin Swift support. I don't know when that'll happen but I expect to it to happen within next couple of months (ofc not a guarantee). So this might mean you have to do some of these changes again. If you want to try the new WIP CMake bootstrapping, run Utilities/new-bootstrap.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good, I'm going to try cross-compiling SPM itself from linux to Android next, so I was going to try the CMake support for that.

Copy link
Contributor

@aciidgh aciidgh left a comment

Choose a reason for hiding this comment

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

This is awesome, thanks!

@aciidgh
Copy link
Contributor

aciidgh commented Nov 22, 2019

@swift-ci smoke test

@@ -1175,6 +1186,10 @@ def main():
# Append the versioning build flags.
build_flags.extend(create_versoning_args(args))

# Don't use GNU strerror_r on Android.
if 'ANDROID_DATA' in os.environ:
build_flags.extend(["-Xswiftc", "-Xcc", "-Xswiftc", "-U_GNU_SOURCE"])
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine for now but this should go to BuildPlan.swift so swiftpm can correct compile itself and other Swift targets for Android.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, _GNU_SOURCE was set for all clang imports for Android in the Swift compiler a year ago, swiftlang/swift@60e2205, so changing that for all packages built for Android could break stuff. This tweak simply unsets it so the GNU strerror_r isn't used when building SPM itself, and avoids those larger issues.

@finagolfin
Copy link
Member Author

I've been backporting this pull and the previous #2396 to the latest Swift 5.1.2 release, only had to make some small tweaks to account for the older toolchain symlinks and cherry-picked two commits from master, which got all the SPM 5.1.2 tests passing natively on Android.

You may want to consider cherry-picking those two commits back to the swift-5.1-branch, to help those porting SPM from the latest source releases, say 5.1.3 next. One commit was caea53a, which I only needed because I enable and run those filesystem tests on Android in my previous pull. Since that changes the implementation of chmod and the test is normally gated, not a big deal if you don't.

The other commit, b876852, would be useful for anyone running the tests and hitting those hardcoded triples for linux x86_64, say if someone was porting SPM to linux ARM. Since it only fixes tests, no risk for the 5.1 branch.

@aciidgh aciidgh merged commit 4adfbcc into swiftlang:master Dec 9, 2019
@aciidgh
Copy link
Contributor

aciidgh commented Dec 9, 2019

5.1 branch is generally not open to take commits. You could nominate changes for the monthly Linux releases but I am not sure what's the process for that. cc @weissi

@finagolfin
Copy link
Member Author

finagolfin commented Dec 10, 2019

OK, I'm not sure what the process is either, just a suggestion based on what commits helped me when porting SPM 5.1.2 to run natively on Android.

Anyway, thanks for merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants