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

update ndk to 17 and switch some packages to clang build #2415

Merged
merged 25 commits into from
May 27, 2018

Conversation

vishalbiswas
Copy link
Contributor

I've tried to update the build process to use Android NDK 17 released on May 8th, 2018 according to the changes mentioned in #1905.

Please note that android/ndk#294 has not been included in r17 and as such we still have to use gcc for emacs and m4 to work around Undefined reference to '__muloti4'.

@Grimler91
Copy link
Member

Grimler91 commented May 10, 2018

I tried an ./clean.sh; ./build-all.sh

Initial findings:

  • libidn now needs termux_step_pre_configure () {LDFLAGS+=" -liconv"}
  • netinet/in.h no longer does typedef uint32_t in_addr_t; which causes In file included from /home/builder/.termux-build/libnl/src/include/netlink-private/netlink.h:32: /home/builder/.termux-build/_lib/17-aarch64-21-v1/bin/../sysroot/usr/include/arpa/inet.h:39: In file included from :/home/builder/.termux-build/libnl/src/lib/addr.c: unknown type name 'in_addr_t' for libnl and probably others. Should we patch this in the headers or patch it in affected packages?

I'll update the list when find more

@vishalbiswas
Copy link
Contributor Author

vishalbiswas commented May 10, 2018

@Grimler91

  • Didn't encounter any errors with libidn
  • Should not be a problem but it is. in_addr_t is defined in bits/in_addr.h which is included by linux/in.h which is included by netinet/in.h which is included by arpa/inet.h. Looking into this. EDIT: libnl uses its own linux/in.h thats why. Manually including bits/in_addr.h should fix this. EDIT 2: Not good enough. libnl's linux/in.h has conflicting definitions for struct in_addr. EDIT 3: home/vishal/.termux-build/_lib/17-arm-21-v1/bin/../sysroot/usr/include/linux/tcp.h:193:27: error: field has incomplete type 'struct sockaddr_storage' as well.

EDIT 4: pushed a fix for libnl. somewhat ugly.

@Grimler91
Copy link
Member

@vishalbiswas

Didn't encounter any errors with libidn

Weird, I'm still seeing it:

/home/builder/.termux-build/_lib/17-aarch64-21-v1/bin/aarch64-linux-android-ld: warning: libiconv.so, needed by ../lib/.libs/libidn.so, not found (try using -rpath or -rpath-link)
../lib/.libs/libidn.so: undefined reference to `iconv_open'                                                 
../lib/.libs/libidn.so: undefined reference to `iconv_close'                                              
../lib/.libs/libidn.so: undefined reference to `iconv'
clang60: error: linker command failed with exit code 1 (use -v to see invocation)                          
Makefile:1444: recipe for target 'idn' failed

Can confirm your libnl patch works.
Next error I encountered was with libpixman. I'll open a PR with a patch in a minute (to your branch).

@Grimler91
Copy link
Member

I get "the same" error with libisl:

/home/builder/.termux-build/_lib/17-aarch64-21-v1/bin/aarch64-linux-android-ld: warning: libgmp.so, needed by ./.libs/libisl.so, not found (try using -rpath or -rpath-link)              
/home/builder/.termux-build/_lib/17-aarch64-21-v1/bin/aarch64-linux-android-ld: warning: libgmp.so, needed by ./.libs/libisl.so, not found (try using -rpath or -rpath-link)
./.libs/libisl.so: undefined reference to `__gmpz_mul'        
./.libs/libisl.so: undefined reference to `__gmpz_get_d'     
./.libs/libisl.so: undefined reference to `__gmpz_swap'
./.libs/libisl.so: undefined reference to `__gmpz_cdiv_q'
...

Solved with LDFLAGS+=" -lgmp" so seems something fundamental has changed.

Henrik Grimler and others added 2 commits May 10, 2018 21:35
@vishalbiswas
Copy link
Contributor Author

vishalbiswas commented May 10, 2018

@Grimler91 that libisl error occurs in aarch64. arm build works fine. working on it
EDIT:
Turns out ndk17 still defaulted to ld.bfd because of https://issuetracker.google.com/issues/70838247

@Grimler91
Copy link
Member

Interesting.

Seems like LDFLAGS+=" -liconv" doesn't help. Building tracepath (which depends on libidn) gives error again:

/home/builder/.termux-build/_lib/17-aarch64-21-v1/bin/aarch64-linux-android-ld: warning: libandroid-support.so, needed by /data/data/com.termux/files/usr/lib/libidn.so, not found (try using -rpath or -rpath-link)
/home/builder/.termux-build/_lib/17-aarch64-21-v1/bin/aarch64-linux-android-ld: warning: libiconv.so, needed by /data/data/com.termux/files/usr/lib/libidn.so, not found (try using -rpath or -rpath-link)
/data/data/com.termux/files/usr/lib/libidn.so: undefined reference to `iconv_open'
/data/data/com.termux/files/usr/lib/libidn.so: undefined reference to `nl_langinfo'
/data/data/com.termux/files/usr/lib/libidn.so: undefined reference to `iconv_close'
/data/data/com.termux/files/usr/lib/libidn.so: undefined reference to `iconv'
clang60: error: linker command failed with exit code 1 (use -v to see invocation)

@vishalbiswas
Copy link
Contributor Author

@Grimler91 pull latest changes, delete existing toolchain and build again.
was probably due to not using gold

@Grimler91
Copy link
Member

Yeah, works now.

Next error is on libzmq:

/home/builder/.termux-build/libzmq/src/src/v1_decoder.cpp:113:28: error: comparison 'unsigned long' > 18446744073709551615 is always false [-Werror,-Wtautological-constant-compare]
    if (payload_length - 1 > std::numeric_limits<size_t>::max ()) {
        ~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

@its-pointless
Copy link
Contributor

grimler add --disable-Werror and libzmq compiles

@its-pointless
Copy link
Contributor

remove gio-gsocket.c.patch from glib not needed?

@vishalbiswas
Copy link
Contributor Author

We also need to switch over most packages to clang now. I have a feeling that they are serious about this being the last release with gcc included.

@Grimler91
Copy link
Member

Grimler91 commented May 11, 2018

@its-pointless for libzmq: I guess we might just as well remove the entire test that fails, since I suppose it will be optimized away anyways if the package is built with --disable-Werror.

edit: --disable-Werror is of course easier though..

@vishalbiswas
Copy link
Contributor Author

Strangely I don't get the error when building for arm

@vishalbiswas
Copy link
Contributor Author

vishalbiswas commented May 11, 2018

I get this when building elinks. Can anybody else try and see if they get the same errors?

lib.o:getifaddrs.c:function getifaddrs2: error: undefined reference to '__errno_location'
lib.o:getifaddrs.c:function getifaddrs2: error: undefined reference to '__errno_location'
lib.o:getifaddrs.c:function getifaddrs2: error: undefined reference to '__errno_location'
lib.o:vs.c:function set_window_title: error: undefined reference to 'stdout'```

@its-pointless
Copy link
Contributor

this ax_cv_PTHREAD_PRIO_INHERIT=no
needs to be added to libpulseaudio configure args as it detects it in libc but its only available at api 28.

@vishalbiswas
Copy link
Contributor Author

vishalbiswas commented May 11, 2018

/home/vishal/.termux-build/ltrace/src/sysdeps/linux-gnu/arm/plt.c:65:32: error: use of undeclared identifier 'SHT_ARM_ATTRIBUTES'
        if (elf_get_section_type(lte, SHT_ARM_ATTRIBUTES, &scn, &shdr) < 0

guess that elf.h won't be going away after all
or should we just include the definition in this package (ltrace) itself as it is the only one I've encountered yet

@Grimler91
Copy link
Member

@vishalbiswas I get no error for elinks when building for aarch64.

@vishalbiswas
Copy link
Contributor Author

@Grimler91 probably occurring because I don't build inside docker. I used to get similar errors previously.

@tomty89
Copy link
Contributor

tomty89 commented May 11, 2018

this ax_cv_PTHREAD_PRIO_INHERIT=no
needs to be added to libpulseaudio configure args as it detects it in libc but its only available at api 28.

Hmm, doesn't the build system of Termux has a general approach that make sure only features of API 21 are available?

@its-pointless
Copy link
Contributor

--disable-gst_v4l2 to be added to gst-plugins-good

@Grimler91
Copy link
Member

Grimler91 commented May 11, 2018

torsocks needs a patch. I've opened a PR.

Edit: Vishal beat me to it.

@tomty89
Copy link
Contributor

tomty89 commented May 14, 2018

@Grimler91 I don't think it really has something to do with the architectures. It's just somehow you have files of libllvm-dev (but not libclang-dev) in the build directories of the 64-bit ones. See my tests here: https://ptpb.pw/2H3a
This is probably partly because our current llvm/clang packaging are broken. See #2282 for details, if interested.
I am not sure if the libobjc2 solution makes sense though. Smells a bit like the opencv/libbboost_python case.

@its-pointless
Copy link
Contributor

k built ruby on arm with clang
going to try emacs now

@its-pointless
Copy link
Contributor

emacs now builds on clang

@its-pointless
Copy link
Contributor

termux_step_pre_configure() {
export LIBS=" -lcompiler_rt-extras"
}
seems to get it to compile

@tomty89
Copy link
Contributor

tomty89 commented May 14, 2018

Makes me wonder if -rtlib=compiler-rt should be used with / instead of it.

@its-pointless
Copy link
Contributor

no all of the compiler_rt stuff neccesary to replace libgcc is not there. Or we could just add all the stuff from libcompiler_rt-extras into libgcc

@its-pointless
Copy link
Contributor

contents of libcompiler_rt-extras
mulodi4.o

fuck it just add that file to libgcc.a and we are done

@its-pointless
Copy link
Contributor

nope this won't work consistently m4 on aarch64 using clang requires __muloti4 which is not in libcompiler_rt-extras.a for aarch64.

@vishalbiswas
Copy link
Contributor Author

vishalbiswas commented May 15, 2018

@its-pointless __muloti4 is not something I'm terribly worried about. If worse comes to worst, we can use an unoptimised version of a similar function.

@Grimler91
Copy link
Member

@tomty89 Is your device aarch64?
Clearly something is wrong with the toolchain or one of the cmake scripts, but I don't see how it's related to our libllvm packaging. The compilation was done in a docker image, libllvm hadn't been compiled prior to building libobjc2, error must be in the ndk or (perhaps more likely) our build-package.sh script.

I believe the libobjc2 solution makes more sense than the libboost_python/opencv solution did. Seems to be several ways to to get these block functions though: libobjc2, blocksruntime. Maybe blocksruntime would make more sense to use.
I've been looking at https://stackoverflow.com/questions/2297765/which-libraries-do-you-need-to-link-against-for-a-clang-program-using-blocks and https://stackoverflow.com/questions/14980402/how-to-make-gnustep-libobjc2-to-work.

@tomty89
Copy link
Contributor

tomty89 commented May 15, 2018

@Grimler91 yes it is:

$ uname -m
aarch64
$

I can't tell for sure if it's exactly the same case as my test, since it's cross-compiling.
But specifically what I meant by broken packaging is that, LLVMExports.cmake and libLLVMDemangle.a should belong to the same subpackage, whether it matters or not in this case.

As for whether the solution makes sense, IMHO we can come to a conclusion in that only if we know what exactly we are using libobjc2 as a supplement to, and why it is needed now. Any -l without concrete ground is irrational.

@Grimler91
Copy link
Member

Where do we get libLLVMDemangle.a from? This file isn't present in ~/lib or ~/.termux-build/_lib/ so the error about it missing makes sense, but it's weird that we don't get the same error on arm or i686 then.

@tomty89

As for whether the solution makes sense, IMHO we can come to a conclusion in that only if we know what exactly we are using libobjc2 as a supplement to, and why it is needed now.

Sure, I should perhaps research the issues more before attacking them.
Android ndk's clang toolchain seem to be missing these symbols related to block functions:

_Block_object_dispose
_NSConcreteStackBlock
_Block_object_assign

They can be found in several libraries: libobjc2, BlocksRuntime, compiler-rt (BlocksRuntime seems to be identical to the compiler-rt code).
So linking against libobjc2 provides these missing symbols and elfutils builds without that error.

@tomty89
Copy link
Contributor

tomty89 commented May 15, 2018

@Grimler91 libLLVMDemangle.a is currently in libclang-dev (a "fake" -dev package, i.e. split manually from libllvm with a subpackage script) while LLVMExports.cmake is currently in libllvm-dev (the "real" -dev package, i.e. split automatically for libllvm)

Both files are products of the llvm source and has nothing to do with clang.

@fornwall
Copy link
Member

@vishalbiswas @its-pointless:

-Oz for arm was fixed https://reviews.llvm.org/rL321996. But I still get #1680 when building with -Oz.

The fix seems to be in r321996, but the NDK r17 changelog says Updated Clang to build 4691093, based on r316199, so I guess we'll have to wait for the next NDK update to get this fix.

@fornwall
Copy link
Member

About gcc->clang transitions: We don't need to have any more of these in this PR, that can be made in separate ones (and some may wait till NDK r18).

@fornwall
Copy link
Member

Oh right I missed that, but still anything linked to it should have their revision bumped.

I can make a script which finds all packages that links against libc++_shared.so and revision bump them. Should I do that or is anyone else working on it?

@its-pointless
Copy link
Contributor

its-pointless commented May 17, 2018 via email

fornwall added a commit that referenced this pull request May 19, 2018
@fornwall
Copy link
Member

@vishalbiswas You can remove gst-plugins-good/build.sh from this PR, it has already been applied there when updating the package!

Besides the version bump of libc++-using packages, is there anything else remaining here that we know of?

This reverts commit 43850b8.
Already applied on base branch.
@vishalbiswas
Copy link
Contributor Author

vishalbiswas commented May 19, 2018

@fornwall

You can remove gst-plugins-good/build.sh from this PR, it has already been applied there when updating the package!

Done!
I don't think anything else is remaining.
EDIT:
Except maybe the llvm triple on arm.
I'll update it right now.

@vishalbiswas vishalbiswas changed the title update ndk to 17 update ndk to 17 and switch some packages to clang build May 19, 2018
@fornwall
Copy link
Member

fornwall commented May 27, 2018

@vishalbiswas Could you fix the conflicts, and I'll merge this PR ASAP (and bump the libc++-using packages on master)?

EDIT: Ok, the merge conflict was trivial, so this is merged now. Great work everyone involved!

@fornwall fornwall merged commit efa8300 into termux:master May 27, 2018
@fornwall
Copy link
Member

The docker image has been updated - run scripts/update-docker.sh to pull it.

@b3nsh4
Copy link

b3nsh4 commented May 27, 2018

@fornwall , @Grimler91 , @xeffyr Can you guys please help me with this, #2451
because I currently stopped my project due to this error. Kindly help !
#2451

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.

6 participants