Upgrade to platform-tools-34.0.5 #132

Biswa96 · 2024-02-06T20:04:27Z

Fixes #131

* adb/0024-Add-explicit-constructor-for-fdevent_event.patch https://android.googlesource.com/platform/packages/modules/adb.git/+/c7bb7813905b5f879dc4c30cac1ee2a64852b8da * base/0004-Move-Theme-Entry-type-definition-up-before-it-is-use.patch https://android.googlesource.com/platform/frameworks/base/+/e8e793a340c87d650a73a29d7915f12e1dc6146b * core/0010-Correct-version-in-which-gettid-was-introduced-to-gl.patch https://android.googlesource.com/platform/system/core.git/+/253445ce3ad507f41c61ebf0f829f75ee2c37509 https://android.googlesource.com/platform/system/core.git/+/8b0160868eefc2997155906aa361470fb9a6c9ab * libziparchive/0002-fix-cxx-narrowing-error.patch https://android.googlesource.com/platform/system/libziparchive/+/035f8d302af88ae2d866c84f5a1528e098e3d1a0

anatol · 2024-02-07T15:54:07Z

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 · 2024-02-07T16:04:55Z

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 · 2024-02-07T17:54:29Z

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 · 2024-02-07T18:27:42Z

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

This fixes the following error in Alpine linux. In file included from /andy/vendor/libbase/abi_compatibility.cpp:20: /andy/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); | ^~~~~~~

This is disabled due to following compiler error. core/libcutils/include/private/fs_config.h:26:10: fatal error: 'linux/capability.h' file not found include <linux/capability.h> ^~~~~~~~~~~~~~~~~~~~

Biswa96 · 2024-02-09T15:21:12Z

Now only the clang errors remain.

munix9 · 2024-02-10T06:08:08Z

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 · 2024-02-10T06:15:07Z

I would suggest temporarily disabling clang builds.

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

anatol · 2024-02-12T05:19:03Z

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 · 2024-02-14T17:04:24Z

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 · 2024-02-14T17:40:57Z

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 · 2024-02-16T01:09:51Z

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 · 2024-02-16T07:30:40Z

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 · 2024-02-17T06:12:09Z

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 · 2024-02-17T06:24:20Z

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 · 2024-02-18T14:51:01Z

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 · 2024-02-18T18:19:35Z

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 · 2024-03-19T05:09:46Z

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 · 2024-03-19T05:11:54Z

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

anatol · 2024-03-19T23:37:08Z

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:

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

I am in favor of # 1.

anatol · 2024-03-20T00:04:50Z

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

anatol · 2024-03-20T00:07:14Z

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 · 2024-03-20T04:38:31Z

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 · 2024-03-20T07:11:26Z

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 · 2024-03-20T07:14:27Z

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

Biswa96 · 2024-03-20T07:35:34Z

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 · 2024-03-22T01:00:01Z

Time for 34.0.5 release?

Biswa96 added 2 commits February 8, 2024 01:26

cmake: Disable e2fsdroid in macOS

721cd7c

This is disabled due to following compiler error. core/libcutils/include/private/fs_config.h:26:10: fatal error: 'linux/capability.h' file not found include <linux/capability.h> ^~~~~~~~~~~~~~~~~~~~

Biswa96 force-pushed the 34.0.5 branch from 9b6cb24 to 721cd7c Compare February 9, 2024 15:11

z3ntu mentioned this pull request Feb 19, 2024

Build with -ftrivial-auto-var-init=zero #133

Closed

Drop static check to unblock clang support

370c94c

Biswa96 marked this pull request as ready for review March 20, 2024 05:03

Biswa96 merged commit 3f3b2d8 into nmeum:master Mar 20, 2024
13 of 14 checks passed

Biswa96 deleted the 34.0.5 branch March 20, 2024 07:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to platform-tools-34.0.5 #132

Upgrade to platform-tools-34.0.5 #132

Biswa96 commented Feb 6, 2024

anatol commented Feb 7, 2024

anatol commented Feb 7, 2024

Biswa96 commented Feb 7, 2024

anatol commented Feb 7, 2024 •

edited

Loading

Biswa96 commented Feb 9, 2024

munix9 commented Feb 10, 2024

Biswa96 commented Feb 10, 2024

anatol commented Feb 12, 2024

anatol commented Feb 14, 2024

Biswa96 commented Feb 14, 2024

anatol commented Feb 16, 2024

Biswa96 commented Feb 16, 2024

anatol commented Feb 17, 2024

anatol commented Feb 17, 2024 •

edited

Loading

Biswa96 commented Feb 18, 2024

anatol commented Feb 18, 2024

anatol commented Mar 19, 2024

Biswa96 commented Mar 19, 2024

anatol commented Mar 19, 2024

anatol commented Mar 20, 2024

anatol commented Mar 20, 2024

Biswa96 commented Mar 20, 2024

anatol commented Mar 20, 2024

anatol commented Mar 20, 2024

Biswa96 commented Mar 20, 2024

anatol commented Mar 22, 2024

Upgrade to platform-tools-34.0.5 #132

Upgrade to platform-tools-34.0.5 #132

Conversation

Biswa96 commented Feb 6, 2024

anatol commented Feb 7, 2024

anatol commented Feb 7, 2024

Biswa96 commented Feb 7, 2024

anatol commented Feb 7, 2024 • edited Loading

Biswa96 commented Feb 9, 2024

munix9 commented Feb 10, 2024

Biswa96 commented Feb 10, 2024

anatol commented Feb 12, 2024

anatol commented Feb 14, 2024

Biswa96 commented Feb 14, 2024

anatol commented Feb 16, 2024

Biswa96 commented Feb 16, 2024

anatol commented Feb 17, 2024

anatol commented Feb 17, 2024 • edited Loading

Biswa96 commented Feb 18, 2024

anatol commented Feb 18, 2024

anatol commented Mar 19, 2024

Biswa96 commented Mar 19, 2024

anatol commented Mar 19, 2024

anatol commented Mar 20, 2024

anatol commented Mar 20, 2024

Biswa96 commented Mar 20, 2024

anatol commented Mar 20, 2024

anatol commented Mar 20, 2024

Biswa96 commented Mar 20, 2024

anatol commented Mar 22, 2024

anatol commented Feb 7, 2024 •

edited

Loading

anatol commented Feb 17, 2024 •

edited

Loading