Closed Biswa96 closed 5 months ago
Macos checks fail because
34.0.5/vendor/selinux/libsepol/include -isystem /usr/local/Cellar/pcre2/10.42/include -D_DARWIN_C_SOURCE -D__DARWIN_C_LEVEL=__DARWIN_C_FULL -O3 -DNDEBUG -std=gnu11 -isysroot /Applications/Xcode_13.2.1.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk -mmacosx-version-min=11.7 -Wno-attributes -MD -MT vendor/CMakeFiles/e2fsdroid.dir/e2fsprogs/contrib/android/e2fsdroid.c.o -MF vendor/CMakeFiles/e2fsdroid.dir/e2fsprogs/contrib/android/e2fsdroid.c.o.d -o vendor/CMakeFiles/e2fsdroid.dir/e2fsprogs/contrib/android/e2fsdroid.c.o -c /Users/runner/work/android-tools/android-tools/android-tools-34.0.5/vendor/e2fsprogs/contrib/android/e2fsdroid.c
In file included from /Users/runner/work/android-tools/android-tools/android-tools-34.0.5/vendor/e2fsprogs/contrib/android/e2fsdroid.c:10:
In file included from /Users/runner/work/android-tools/android-tools/android-tools-34.0.5/vendor/e2fsprogs/contrib/android/perms.h:53:
/Users/runner/work/android-tools/android-tools/android-tools-34.0.5/vendor/core/libcutils/include/private/fs_config.h:26:10: fatal error: 'linux/capability.h' file not found
#include <linux/capability.h>
^~~~~~~~~~~~~~~~~~~~
1 error generated.
it looks like this functionality need to be disabled at macos platform.
Alpine GCC fails with
In file included from /__w/android-tools/android-tools/android-tools-34.0.5/vendor/extras/ext4_utils/wipe.cpp:21:
/__w/android-tools/android-tools/android-tools-34.0.5/vendor/libbase/include/android-base/file.h:105:71: error: 'off64_t' has not been declared
105 | bool ReadFullyAtOffset(borrowed_fd fd, void* data, size_t byte_count, off64_t offset);
| ^~~~~~~
/__w/android-tools/android-tools/android-tools-34.0.5/vendor/libbase/include/android-base/file.h:108:78: error: 'off64_t' has not been declared
108 | bool WriteFullyAtOffset(borrowed_fd fd, const void* data, size_t byte_count, off64_t offset);
| ^~~~~~~
In file included from /usr/include/c++/13.2.1/bits/cxxabi_init_exception.h:38,
from /usr/include/c++/13.2.1/bits/exception_ptr.h:36,
from /usr/include/c++/13.2.1/exception:164,
from /usr/include/c++/13.2.1/ios:41,
from /usr/include/c++/13.2.1/ostream:40,
from /usr/include/c++/13.2.1/iostream:41,
from /__w/android-tools/android-tools/android-tools-34.0.5/vendor/core/include/utils/String16.h:20,
from /__w/android-tools/android-tools/android-tools-34.0.5/vendor/core/libutils/String16.cpp:17:
Alpine uses musl library. Looking at the musl sourcecode here https://git.musl-libc.org/cgit/musl/tree/include/sys/types.h I see that off64_t requires _LARGEFILE64_SOURCE
define, i.e. make needs to be called with -D_LARGEFILE64_SOURCE
.
make needs to be called with
-D_LARGEFILE64_SOURCE
.
I am not sure if that will cause any further issue. Should it be enabled for all architectures and all libc?
I am not sure if that will cause any further issue. Should it be enabled for all architectures and all libc?
There should not be. It is a compile define that enables a few extra functionalities.
In fact we already enabled the 64bit file functions here https://github.com/nmeum/android-tools/commit/b7ab657f46caf6bf69e81c7d47a5130d6c26c401 but musl uses a define with different name for some reason.
I think it will be safe to add the required define in addition to what we already have in vendor/CMakeLists.txt
Now only the clang errors remain.
The build problems apparently only occur with more recent clang versions.
I can't contribute directly to the solution at the moment, but if this is the stopper for the release, I would suggest temporarily disabling clang builds.
The package managers of the distris can switch to gcc for the time being.
I would suggest temporarily disabling clang builds.
It is not required to disable anything in this repository. The CI would be red only.
Disabling clang is a big restriction. I would like to understand better what is going on with the compiler and fix/workaround the problem if possible.
I tried to reproduce the problem with godbolt but Block
class compiles fine https://godbolt.org/z/Ybecbohns
One might try to revert this upstream commit and see if it makes clang happy https://android.googlesource.com/platform/packages/modules/adb/+/45720fdaea751c63081541b6c18ab512becd32a4%5E%21/
One might try to revert this upstream commit and see if it makes clang happy
Nope! clang becomes angry again after reverting that commit
In file included from android-tools/vendor/adb/client/adb_client.cpp:20:
In file included from android-tools/vendor/adb/client/adb_client.h:23:
In file included from android-tools/vendor/adb/adb.h:30:
In file included from android-tools/vendor/adb/socket.h:29:
android-tools/vendor/adb/types.h:101:9: error: static assertion failed due to requirement 'std::is_standard_layout<Block>()'
static_assert(std::is_standard_layout<Block>());
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
I also looked at the flags used for compilation here https://android.googlesource.com/platform/packages/modules/adb/+/refs/heads/main/Android.bp
The only unusual item I spotted is use of cpp_std: "experimental",
. Does it mean the newer cpp standard setting? If yes we can try to compile the module with -std=gnu++2c
?
If yes we can try to compile the module with
-std=gnu++2c
?
The vendor/cmakelists.txt sets default C++ 20 standard. I have tried with C++ 23 (gnu++2b) in that cmake file but got same compiler error in CI. clang 16 does not have -std=gnu++2c
option.
Here is a repro case https://godbolt.org/z/WGdq9jfsd
If flag -stdlib=libc++
presents then it compiles fine. Without that flag it fails with
error: static assertion failed due to requirement 'std::is_standard_layout<Block>()'
static_assert(std::is_standard_layout<Block>());
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
I tried to set -stdlib=libstdc++
and it also fails.
So one solution is to use the same stdlib as AOSP in case if clang+*nux is used. We do not need this flag for clang+macos.
And the whole reprocase is reduced to
#include <string.h>
#include <algorithm>
#include <memory>
#include <type_traits>
#include <utility>
#include <vector>
struct Block {
std::unique_ptr<char[]> data_;
};
static_assert(std::is_standard_layout<Block>());
The default libstdc++
does not provide is_standard_layout for its implementation of std::unique_ptr
for some reason. While libc++
does work.
PS it could be related to this GCC bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104419
Did you test with clang 17 or greater? I was wondering if ArchLinux could provide clang 17 for testing. clang 17 supports gnu++2c option.
It fails the same way with clang 17 and trunk
version. You can check godbolt and choose different compilers/options.
The problem is really is in different implementation of unique_ptr
at libc++ vs libstdc++
Let's get this PR done.
There is only one step to be completed here. That is the incompatibility between libstdc++
implementation of std::unique_ptr
and clang
. To resolve it the build system needs to enforce using libc++
if clang
is used.
I am not sure how to solve the error. Please feel free to add necessary changes.
Okay so I switched to clang's libc++
with following patch:
diff --git a/vendor/CMakeLists.txt b/vendor/CMakeLists.txt
index bcabe7e..09da9db 100644
--- a/vendor/CMakeLists.txt
+++ b/vendor/CMakeLists.txt
@@ -16,6 +16,12 @@ if(APPLE)
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -D_DARWIN_C_SOURCE -D__DARWIN_C_LEVEL=__DARWIN_C_FULL")
endif()
+if((CMAKE_CXX_COMPILER_ID MATCHES "Clang"))
+ # GNU libstd is not fully compatible with Clang, see https://github.com/nmeum/android-tools/pull/132#issuecomment-1949759598
+ add_compile_options($<$<COMPILE_LANG_AND_ID:CXX,Clang>:-stdlib=libc++>)
+ add_link_options($<$<LINK_LANGUAGE:CXX>:-lc++>)
+endif()
+
# Android seems to use various attributes supported by clang but not by
# GCC which causes it to emit lots of warnings. Since these attributes
# don't seem to effect runtime behaviour simply disable the warnings.
It fixes the compilation error above. BUT, it adds this linker error
/usr/bin/ld: vendor/libliblpdump.a(dynamic_partitions_device_info.pb.cc.o):(.data.rel.ro+0x1d0): undefined reference to `google::protobuf::Message::GetTypeName() const'
/usr/bin/ld: vendor/libliblpdump.a(dynamic_partitions_device_info.pb.cc.o):(.data.rel.ro+0x1f0): undefined reference to `google::protobuf::Message::InitializationErrorString() const'
/usr/bin/ld: vendor/libliblpdump.a(dynamic_partitions_device_info.pb.cc.o):(.data+0x10): undefined reference to `google::protobuf::internal::fixed_address_empty_string'
googling around I found this SO message https://stackoverflow.com/a/50836934
I had a similar problem, that was caused by the fact that I compiled the protobuf library with GNU's libstdc++, but in the application I was using Clang's libc++, and the two don't work together.
i.e. exactly what we try to do here. We try to link our tools against libc++
while dependent libs from Arch are using libstdc++
by default. Hence the error.
If my hypothesis is true then we really have 2 options here:
I am in favor of # 1.
Now macos-13
fails with
==> Pouring python@3.12--3.12.2_1.ventura.bottle.tar.gz
Error: The `brew link` step did not complete successfully
The formula built, but is not symlinked into /usr/local
Could not symlink bin/2to3
Target /usr/local/bin/2to3
already exists. You may want to remove it:
rm '/usr/local/bin/2to3'
To force the link and overwrite all conflicting files:
brew link --overwrite python@3.12
To list all files that would be deleted:
brew link --overwrite python@3.12 --dry-run
Possible conflicting files are:
/usr/local/bin/2to3 -> /Library/Frameworks/Python.framework/Versions/3.12/bin/2to3
hm...
As well as this macos
warning looks suspicious:
==> Upgrading 7 dependents of upgraded formulae:
Disable this behaviour by setting HOMEBREW_NO_INSTALLED_DEPENDENTS_CHECK.
Hide these hints with HOMEBREW_NO_ENV_HINTS (see `man brew`).
composer 2.7.1 -> 2.7.2, git 2.43.2 -> 2.44.0, glib 2.78.4 -> 2.80.0, harfbuzz 8.3.0 -> 8.3.1, kotlin 1.9.22 -> 1.9.23, php 8.3.3 -> 8.3.4, selenium-server 4.17.0 -> 4.18.1
@Biswa96 is it something expected?
We can ignore the macos 13 issue. The error comes from installing python 3.12. The same issue happened with python 3.11 but was fixed later https://github.com/actions/runner-images/issues/6459
We can ignore the macos 13 issue. The error comes from installing python 3.12. The same issue happened with python 3.11 but was fixed later actions/runner-images#6459
It is OK to ignore it briefly. But there need to be a fix for this runner; otherwise macos-13
needs to be disabled.
Looks good to me. I will be happy to see this PR merged.
It is OK to ignore it briefly. But there need to be a fix for this runner; otherwise
macos-13
needs to be disabled.
Probably, there are working on this https://github.com/actions/runner-images/commit/250c5145f6085d9e70aa11158458854dd7043c6c
Time for 34.0.5 release?
Fixes #131