Skip to content
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

obscure failure from Configure if $ANDROID_NDK has a trailing / #6917

Closed
enh opened this issue Aug 11, 2018 · 11 comments
Closed

obscure failure from Configure if $ANDROID_NDK has a trailing / #6917

enh opened this issue Aug 11, 2018 · 11 comments

Comments

@enh
Copy link

enh commented Aug 11, 2018

with $ANDROID_NDK set to /usr/local/google/home/enh/Downloads/android-ndk-r17b/ and $PATH set to /usr/local/google/home/enh/Downloads/android-ndk-r17b//toolchains/llvm/prebuilt/linux-x86_64/bin/:..., .Configure fails in this non-obvious way:

$ ./Configure android-arm64 -D__ANDROID_API__=26 shared
Configuring OpenSSL version 1.1.1-pre8 (0x10101008L) for android-arm64
Using os-specific seed configuration
no NDK aarch64-linux-android-gcc on $PATH at (eval 8) line 87.

this works:

            if (which("clang") =~ m|^$ndk.*/prebuilt/([^/]+)/|) {

as does this:

            if (which("clang") =~ m|^$ndk/?.*/prebuilt/([^/]+)/|) {

as do various ways of stripping a trailing / from $ndk. but my Perl's not good enough to know what the preferred way of doing that looks like, so i'm filing a bug rather than sending a pull request since i assume you'll want to rewrite whatever i do anyway :-)

@levitte
Copy link
Member

levitte commented Aug 11, 2018

Could you try this patch?

diff --git a/Configurations/15-android.conf b/Configurations/15-android.conf
index ddd642a117..89c5d0ad89 100644
--- a/Configurations/15-android.conf
+++ b/Configurations/15-android.conf
@@ -4,6 +4,8 @@
 # comments below...
 
 {
+    use File::Spec::Functions;
+
     my $android_ndk = {};
     my %triplet = (
         arm    => "arm-linux-androideabi",
@@ -22,6 +24,7 @@
 
             my $ndk = $ENV{ANDROID_NDK};
             die "\$ANDROID_NDK is not defined"  if (!$ndk);
+            $ndk = canonpath($ndk);
             die "\$ANDROID_NDK=$ndk is invalid" if (!-d "$ndk/platforms");
 
             my $ndkver = undef;

@enh
Copy link
Author

enh commented Aug 11, 2018

yes, that works for me.

is which(1) guaranteed to return a canonical path, or do we need to call canonpath on the result of which("clang") too (in case someone has their NDK installed on the other side of a symbolic link)?

another option that occurred to me is saying /+ rather than just / after $ndk:

            if (which("clang") =~ m|^$ndk.*/+prebuilt/([^/]+)/|) {

but calling canonpath on both sides does sound like the most bullet-proof of all. i'm sure someone out there is using symlinks :-)

btw, i'm assuming you're not using a standalone toolchain on purpose because you don't want to require so much disk space? in NDK r19 we'll move a lot of this stuff into the clang driver, so basically every toolchain is a standalone toolchain. at that point most of this manual configuration should disappear.

@levitte
Copy link
Member

levitte commented Aug 11, 2018

which is defined in Configure in File::Spec terms, so yeah, it should return a canonical path.

Regarding the rest, I defer to @dot-asm

@dot-asm
Copy link
Contributor

dot-asm commented Aug 12, 2018

It's sufficient to remove trailing slash[es] in $ndk early enough. Modifying individual expression[s] won't do, because you either make an assumption that there always is redundant / or allow for undesired ways to trick it into other obscure failures. One can probably argue that it would be more pedagogical to show original $ndk in error message, so that user sees what is actually passed. In other words

            $ndk = canonpath($ndk);
            die "\$ANDROID_NDK=$ENV{ANDROID_NDK} is invalid" if (!-d "$ndk/platforms");

As for standalone toolchain. No, we are not using it. I don't even know if it would work with our Configure.

in NDK r19 we'll move a lot of this stuff into the clang driver

I don't quite follow what stuff. It sounds like there will be $triarch-clang. Is it correct? In which case it would be more than appropriate to adjust 15-android.conf...

levitte added a commit to levitte/openssl that referenced this issue Aug 12, 2018
Extra slashes in paths are permissible in Unix-like platforms...
however, when compared with the result from 'which', which returns
canonical paths, the comparison might fail even though the compared
paths may be equivalent.  We make the NDK path canonical internally to
ensure the equivalence compares as equal, at least for the most
trivial cases.

Fixes openssl#6917
@levitte
Copy link
Member

levitte commented Aug 12, 2018

It sounds like there will be $triarch-clang. Is it correct?

No. The way I've understood things, clang is a cross compiler natively, so there are no separate binaries. Instead, there's the -target option that takes $triarch as argument. So if you want a mechanism similar to $triarch-gcc, the answer is clang -target $triarch.

Supporting that requires a bit of work (i.e. we must work a bit on how we treat $(CROSS_COMPILE))

https://clang.llvm.org/docs/CrossCompilation.html

@dot-asm
Copy link
Contributor

dot-asm commented Aug 12, 2018

if you want a mechanism similar to $triarch-gcc, the answer is clang -target $triarch.

That's what 15-android.conf does already. It zeros $(CROSS_COMPILE) prefix and switches to -target if clang is used. This has unpleasant side effect. That user also has to have suitable unprefixed ar and ranlib on the $PATH. And the actual trouble is that it's not same path as clang, which leaves room to misunderstandings and screwups. If there was $triarch-clang (which can as well be a link to unprefixed clang) next to $triarch-ar and $triarch-ranlib, it would make configuration more robust. This was the actual question. Will there be $triarch-clang next to $triarch-ar and $triarch-ranlib. The question is triggered by the fact that referred link to standalone toolchain mentioned $triarch-clang, $triach-clang++, $triarch-ar, etc. And it was suggested that NDK 19 will be standalone-like.

@levitte
Copy link
Member

levitte commented Aug 12, 2018

Is there a reason why calng -target $triarch shouldn't work with $triarch-ar, $triarch-ranlib, etc? Assuming correct implementations, of course...

@dot-asm
Copy link
Contributor

dot-asm commented Aug 12, 2018

Is there a reason why calng -target $triarch shouldn't work with $triarch-ar, $triarch-ranlib, etc?

You mean that Configure would set AR/RANLIB explicitly to $triarch-ar/ranlib? Recall that $(CROSS_COMPILE) is zeroed. Also note that makefile template doesn't allow to override $(CROSS_COMPILE) prefix. So it would be e.g. $(CROSS_COMPILE)arm-linux-androideabi-ar. Ugliness is not fatal of course. It's an option, but it would still require that user makes two modifications to $PATH. And second one would still be open to misinterpretations...

@dot-asm
Copy link
Contributor

dot-asm commented Aug 12, 2018

Just in case. Just like it would be useful if there were $triarch-ar and $triarch-ranlib next to $triarch-clang, it would be as useful if there were multiarch ar and ranlib next to NDK clang.

@enh
Copy link
Author

enh commented Aug 12, 2018

which is defined in Configure in File::Spec terms, so yeah, it should return a canonical path.

ah, i assumed it was a Perl built-in. sgtm then, thanks!

moving on to my accidental derailment of this bug...

The way I've understood things, clang is a cross compiler natively, so there are no separate binaries.

correct. (plus you'll be able to specify the target API level in the "triple", and a lot of the stuff you currently have to do manually based on API level, including things like -fPIE, will just be done by the driver.)

Just in case. Just like it would be useful if there were $triarch-ar and $triarch-ranlib next to $triarch-clang, it would be as useful if there were multiarch ar and ranlib next to NDK clang.

as a stopgap we plan to have $triarch-gcc that calls the multi-arch clang, but the llvm binaries and the binutils binaries aren't likely to live in the same directory (for a combination of historical reasons, limitations of the license compliance checking stuff not liking different licenses in the same directory, and everyone having to change their custom build systems if things move around).

long-term things are likely to move in the opposite direction, with us going from N $target_triple-ar binaries in a separate directory to a multi-arch llvm-ar binary in the llvm directory next to the multi-arch clang.

that's a long way out, though. we've only just moved the Android OS build over to lld (the llvm ld), and we've only just started to switch to the llvm-* tools instead of binutils. even when that's done for the OS, there will be a phased transition in the NDK, similar to the GCC -> clang and gnustl/stlport -> libc++ transitions.

hopefully https://android.googlesource.com/platform/ndk/+/master/docs/Roadmap.md#easier-access-to-common-open_source-libraries (which is what i'm working on now) should mean that fewer developers actually have to build from scratch anyway and can have a more apt-get openssl-dev kind of experience...

@levitte
Copy link
Member

levitte commented Aug 12, 2018

Side note: incidently, there is a module File::Which, which also produces results with the help of File::Spec. The only reason we don't use it is that it's not part of core perl.

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

No branches or pull requests

3 participants