Skip to content

Android: fix ABI triple detection, disable C++ modules flags, and bring back bootstrap script tweaks #2466

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 17, 2019

Conversation

finagolfin
Copy link
Member

This pull redoes the previous tweaks for the old non-CMake bootstrap script. Since the new bootstrap script rebuilds llbuild by using its Package.swift, rather than the prior approach of linking against the llbuild already built by the Swift build-script, I had problems when building llbuild with SPM because of the C++ modules support.

Removing those flags, as for Windows, gets llbuild and SPM built, with some of those BuildPlanTests now failing, as they explicitly look for that flag.

In the process, I discovered that the isAndroid() function I added earlier wasn't working right, as it would only detect androideabi and not android. As the two appear to be equivalent, with LLVM just turning the former into the latter, I had the SPM Triple parser do the same.

I also had to modify llbuild's Package.swift to link in ncurses on Android:

diff --git a/Package.swift b/Package.swift
index 6141342..6a20665 100644
--- a/Package.swift
+++ b/Package.swift
@@ -150,7 +150,7 @@ let package = Package(
             name: "llvmSupport",
             dependencies: ["llvmDemangle"],
             path: "lib/llvm/Support",
-            linkerSettings: [.linkedLibrary("ncurses", .when(platforms: [.linux, .macOS]))]
+            linkerSettings: [.linkedLibrary("ncurses", .when(platforms: [.linux, .macOS, .android]))]
         ),
     ],
     cxxLanguageStandard: .cxx14

which then required changing the SPM version where that's supported locally, so llbuild's SPM manifest could use it:

diff --git a/Sources/PackageDescription4/SupportedPlatforms.swift b/Sources/PackageDescription4/SupportedPlatforms.swift
index 06439ebe..8bd41f13 100644
--- a/Sources/PackageDescription4/SupportedPlatforms.swift
+++ b/Sources/PackageDescription4/SupportedPlatforms.swift
@@ -39,7 +39,7 @@ public struct Platform: Encodable {
     public static let windows: Platform = Platform(name: "windows")
 
     /// The Android platform
-    @available(_PackageDescription, introduced: 5.2)
+    @available(_PackageDescription, introduced: 5.0)
     public static let android: Platform = Platform(name: "android")
 }
 

Hopefully, that won't be necessary once Swift 5.2 ships though.

bring over tweaks from old bootstrap script to new one.
@finagolfin finagolfin requested a review from aciidgh as a code owner December 17, 2019 16:04
@@ -61,7 +61,7 @@ public struct Triple: Encodable {

public enum ABI: String, Encodable {
case unknown
case android = "androideabi"
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 raw value was added years back and never used until my recent native Android pulls, so should be fine to change.

@@ -232,7 +232,14 @@ def get_build_target(args):
if platform.system() == 'Darwin':
return "x86_64-apple-macosx"
elif platform.system() == 'Linux':
if platform.machine() == 'x86_64':
if 'ANDROID_DATA' in os.environ:
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey, wanna try using the new swift -print-target-info to get the triple automatically?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll try it out.

if platform.system() == 'Linux':
build_flags.extend(["-Xlinker", "-rpath=$ORIGIN/../lib/swift/linux"])
Copy link
Contributor

Choose a reason for hiding this comment

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

We can probably drive this based on swift -print-target-info as well 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, ‘-print-target-info’ indicates whether one should use rpaths

Copy link
Member Author

Choose a reason for hiding this comment

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

@aciidb0mb3r, do you mean extracting the relative path from the provided runtimeLibraryPaths instead of hardcoding it for each platform here? Because librariesRequireRPath is currently useless on non-Darwin platforms, always returning false.

Copy link
Contributor

Choose a reason for hiding this comment

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

Exacting the "linux" or "android" bit from the triple should be good enough, I think? @DougGregor or @brentdax might have better ideas though.

Anyway, I don't want to block your PR on this stuff so I am happy to take this patch and perhaps you can try swift -print-target-info in a follow on patch 😃

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'll try it in my next pull.

@aciidgh
Copy link
Contributor

aciidgh commented Dec 17, 2019

@swift-ci smoke test

@aciidgh aciidgh merged commit 00c7768 into swiftlang:master Dec 17, 2019
@finagolfin finagolfin deleted the droid branch December 17, 2019 22:05
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.

3 participants