nmeum / android-tools

Unoffical CMake-based build system for android command line utilities
Apache License 2.0
177 stars 51 forks source link

Upgrade to platform-tools-34.0.4 #121

Closed Biswa96 closed 1 year ago

Biswa96 commented 1 year ago

This commit may introduce that issue https://android.googlesource.com/platform/packages/modules/adb.git/+/78d34d8f714f9c78b6b628417cfb690d8fba3900

anatol commented 1 year ago

Thanks for creating the PR.

As of the moment compilation failure - the documentation says construct_at requires header <memory> to be included. https://en.cppreference.com/w/cpp/memory/construct_at

So the file need to be patched and the required header need to be added at the top of the file.

Biswa96 commented 1 year ago

...the documentation says construct_at requires header to be included.

I have tried to include <memory> header in fdevent_epoll.cpp file. But it fails with same compiler error in my archlinux system.

munix9 commented 1 year ago

Strange, on openSUSE Tumbleweed it compiles without problems with clang (github actions and local build). I am still testing the local build with Leap 15.4 and 15.5.

anatol commented 1 year ago

weirdly enough this PR build fine at my Arch Linux machine.

I am using

cmake \
    -DCMAKE_INSTALL_PREFIX=/usr \
    -DCMAKE_BUILD_TYPE=Release \
    -DCMAKE_CXX_FLAGS="$CXXFLAGS" \
    -DCMAKE_C_FLAGS="$CFLAGS" \
    -DCMAKE_FIND_PACKAGE_PREFER_CONFIG=ON \
    -Dprotobuf_MODULE_COMPATIBLE=ON \
    -G Ninja -S . -B build
ninja -C build

And my local CFLAGS CXXFLAGS are empty. What flags are used by the builder?

Biswa96 commented 1 year ago

You have set the compiler also, like CC=clang CXX=clang++ cmake -B build

anatol commented 1 year ago

Thank you @Biswa96 now I see it. It looks like it comes from this piece of code:

LOG(DEBUG) << dump_fde(fde) << " got events " << std::hex << std::showbase << events;
auto& fde_event = fde_events.emplace_back(fde, events);
event_map[fde] = &fde_event;
fde->last_active = post_poll;
In file included from /home/anatol/sources/android-tools/vendor/adb/fdevent/fdevent_epoll.cpp:17:
In file included from /home/anatol/sources/android-tools/vendor/adb/fdevent/fdevent_epoll.h:21:
In file included from /home/anatol/sources/android-tools/vendor/adb/sysdeps.h:30:
In file included from /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/13.2.1/../../../../include/c++/13.2.1/string:54:
In file included from /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/13.2.1/../../../../include/c++/13.2.1/bits/basic_string.h:39:
In file included from /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/13.2.1/../../../../include/c++/13.2.1/ext/alloc_traits.h:34:
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/13.2.1/../../../../include/c++/13.2.1/bits/alloc_traits.h:539:4: error: no matching function for call to 'construct_at'
          std::construct_at(__p, std::forward<_Args>(__args)...);
          ^~~~~~~~~~~~~~~~~
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/13.2.1/../../../../include/c++/13.2.1/bits/vector.tcc:117:21: note: in instantiation of function template specialization 'std::allocator_traits<std::allocator<fdevent_event>>::construct<fdevent_event, fdevent *, unsigned int &>' requested here
            _Alloc_traits::construct(this->_M_impl, this->_M_impl._M_finish,
                           ^
/home/anatol/sources/android-tools/vendor/adb/fdevent/fdevent_epoll.cpp:180:36: note: in instantiation of function template specialization 'std::vector<fdevent_event>::emplace_back<fdevent *, unsigned int &>' requested here
                        fde_events.emplace_back(&fde, events);
                                   ^
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/13.2.1/../../../../include/c++/13.2.1/bits/stl_construct.h:94:5: note: candidate template ignored: substitution failure [with _Tp = fdevent_event, _Args = <fdevent *, unsigned int &>]: no matching constructor for initialization of 'fdevent_event'
    construct_at(_Tp* __location, _Args&&... __args)
    ^
2 errors generated.

It looks like another C++ quirk.

munix9 commented 1 year ago

Tumbleweed uses clang 16, so yes, looks like a problem with clang < 16.

Biswa96 commented 1 year ago

I have asked about this issue in upstream https://android-review.googlesource.com/c/platform/packages/modules/adb/+/2592785

colincross commented 1 year ago

That commit uses a very recent feature from C++20 (https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p0960r3.html). We saw some incompatibilities in the Android tree with even slightly older versions of clang than we currently use. I've uploaded https://android-review.googlesource.com/c/platform/packages/modules/adb/+/2703715 to add an explicit constructor, which should fix your build.

Biswa96 commented 1 year ago

Thank you for the quick response. I have verified that the patch fixes compilation with clang 15.

anatol commented 1 year ago

Thank you for your great work!

Biswa96 commented 1 year ago

@anatol how do you get the notification for the platform-tools update?