Skip to content

Upgrade to platform-tools-34.0.4#121

Merged
anatol merged 3 commits intonmeum:masterfrom
Biswa96:34.0.4
Aug 10, 2023
Merged

Upgrade to platform-tools-34.0.4#121
anatol merged 3 commits intonmeum:masterfrom
Biswa96:34.0.4

Conversation

@Biswa96
Copy link
Collaborator

@Biswa96 Biswa96 commented Aug 10, 2023

No description provided.

@Biswa96
Copy link
Collaborator Author

Biswa96 commented Aug 10, 2023

  • Bad news: I can not figure out how to fix the clang build failure.
  • Good news: clang build fails for all OSes.

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

@anatol
Copy link
Collaborator

anatol commented Aug 10, 2023

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
Copy link
Collaborator Author

Biswa96 commented Aug 10, 2023

...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
Copy link
Contributor

munix9 commented Aug 10, 2023

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
Copy link
Collaborator

anatol commented Aug 10, 2023 •
edited
Loading

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
Copy link
Collaborator Author

Biswa96 commented Aug 10, 2023

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

@anatol
Copy link
Collaborator

anatol commented Aug 10, 2023

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
Copy link
Contributor

munix9 commented Aug 10, 2023

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

@Biswa96
Copy link
Collaborator Author

Biswa96 commented Aug 10, 2023

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

@colincross
Copy link

colincross commented Aug 10, 2023

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
Copy link
Collaborator Author

Biswa96 commented Aug 10, 2023

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

@anatol anatol merged commit 48f85eb into nmeum:master Aug 10, 2023
@anatol
Copy link
Collaborator

anatol commented Aug 10, 2023

Thank you for your great work!

@Biswa96 Biswa96 deleted the 34.0.4 branch August 10, 2023 20:23
@Biswa96
Copy link
Collaborator Author

Biswa96 commented Aug 12, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Biswa96 @anatol @munix9 @colincross