Upgrade to platform-tools-34.0.5

[Biswa96]

Fixes #131

[anatol]

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.

[anatol]

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.

[Biswa96]

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?

[anatol]

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 b7ab657 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

[Biswa96]

Now only the clang errors remain.

[munix9]

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.

[Biswa96]

I would suggest temporarily disabling clang builds.

It is not required to disable anything in this repository. The CI would be red only.

[anatol]

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

[anatol]

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/

[Biswa96]

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>());
^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[anatol]

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?

[Biswa96]

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.

[anatol]

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.

[anatol]

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

[Biswa96]

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.

[anatol]

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++

[anatol]

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.

[Biswa96]

I am not sure how to solve the error. Please feel free to add necessary changes.

[anatol]

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:

1. Drop the static checks that cause the compile error
2. Drop support for clang compiler entirely

I am in favor of # 1.

[anatol]

Now macos-13 fails with

==> Pouring [email protected]_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 [email protected]

To list all files that would be deleted:
  brew link --overwrite [email protected] --dry-run

Possible conflicting files are:
/usr/local/bin/2to3 -> /Library/Frameworks/Python.framework/Versions/3.12/bin/2to3

hm...

[anatol]

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?

[Biswa96]

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

[anatol]

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.

[anatol]

Looks good to me. I will be happy to see this PR merged.

[Biswa96]

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 actions/runner-images@250c514

[anatol]

Time for 34.0.5 release?