nmeum / android-tools

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

Compilation issue on openSUSE Tumbleweed #119

Closed paolostivanin closed 1 year ago

paolostivanin commented 1 year ago

OS: openSUSE Tumbleweed Compiler: gcc13-13.1.1+git7364-1.2 / clang16-16.0.6-1.1 Protobuf: 22.5

After the update to Protobuf 22.5, the package android-tools fails to compile:

[  128s] /home/abuild/rpmbuild/BUILD/android-tools-34.0.1/vendor/extras/libjsonpb/parse/jsonpb.cpp:43:27: warning: 'JsonOptions' is deprecated: use JsonPrintOptions instead [-Wdeprecated-declarations]
[  128s]   google::protobuf::util::JsonOptions options;
[  128s]                           ^
[  128s] /usr/include/google/protobuf/util/json_util.h:45:19: note: 'JsonOptions' has been explicitly marked deprecated here
[  128s] using JsonOptions ABSL_DEPRECATED("use JsonPrintOptions instead") =
[  128s]                   ^
[  128s] /usr/include/absl/base/attributes.h:674:49: note: expanded from macro 'ABSL_DEPRECATED'
[  128s] #define ABSL_DEPRECATED(message) __attribute__((deprecated(message)))
[  128s]                                                 ^
[  128s] /home/abuild/rpmbuild/BUILD/android-tools-34.0.1/vendor/extras/libjsonpb/parse/jsonpb.cpp:54:52: error: no member named 'as_string' in 'std::basic_string_view<char>'
[  128s]     return MakeError<std::string>(status.message().as_string());
[  128s]                                   ~~~~~~~~~~~~~~~~ ^
[  128s] /home/abuild/rpmbuild/BUILD/android-tools-34.0.1/vendor/extras/libjsonpb/parse/jsonpb.cpp:71:55: error: no member named 'as_string' in 'std::basic_string_view<char>'
[  128s]     return MakeError<std::monostate>(status.message().as_string());
[  128s]                                      ~~~~~~~~~~~~~~~~ ^
[  128s] 1 warning and 2 errors generated.
munix9 commented 1 year ago

This is already solved for the official openSUSE Tumbleweed packages, protobuf < 22.5 is required. I still need to do some tests and then it will go to hardware/android-tools and later to Factory/TW.

https://build.opensuse.org/package/show/home:munix9/android-tools

If you absolutely want to use protobuf 22.5, you can use the patch from arch, but it won't work for Leap. https://gitlab.archlinux.org/archlinux/packaging/packages/android-tools/-/blob/main/protobuf-23.patch

paolostivanin commented 1 year ago

@munix9 I've created https://build.opensuse.org/request/show/1096802/changes to apply the patch only for TW

munix9 commented 1 year ago

Yes, thanks, but it doesn't really make much sense currently because protobuf < 22.5 is still offered for Tumbleweed. With a BuildRequires: pkgconfig(protobuf) < 22.5 the problem is already solved.

munix9 commented 1 year ago

BTW: The patch should also take into account older protobuf versions, e.g.:

...
#if GOOGLE_PROTOBUF_VERSION < 3016000
    return MakeError<std::string>(status.error_message().as_string());
#elif GOOGLE_PROTOBUF_VERSION < 4022005
    return MakeError<std::string>(status.message().as_string());
#else
    return MakeError<std::string>(std::string(status.message()));
#endif
...
Biswa96 commented 1 year ago

Should the version check be 4022000 ? Because the BinaryToJsonString() function was changed in 22.0-rc1 version in protobuf.

munix9 commented 1 year ago

I have the value from the current openSUSE Tumbleweed package:

$ grep -r GOOGLE_PROTOBUF_VERSION /usr/include/google/
/usr/include/google/protobuf/stubs/common.h:#define GOOGLE_PROTOBUF_VERSION 4022005

$ rpm -qf /usr/include/google/protobuf/stubs/common.h
protobuf-devel-22.5-4.1.x86_64

It may well be a different value, but unfortunately I can't test that.

By the way, the find_package(Protobuf CONFIG REQUIRED) from the patch does not work with older protobuf versions either (at least under Leap), the REQUIRED would have to be removed. But this is also not tested in detail.

Biswa96 commented 1 year ago

If I tried to build the latest master branch I get the following linking error in ArchLinux. I am not sure if it is related to protobuf change. Any idea?

/usr/bin/ld: vendor/libliblpdump.a(dynamic_partitions_device_info.pb.cc.o): undefined reference to symbol '_ZN4absl12lts_2023012512log_internal21CheckOpMessageBuilder7ForVar2Ev'
/usr/bin/ld: /usr/lib/libabsl_log_internal_check_op.so.2301.0.0: error adding symbols: DSO missing from command line
munix9 commented 1 year ago

Build on openSUSE Tumbleweed with /usr/lib64/libprotobuf.so (found version "3.21.12") works (protobuf21-devel 21.12).

Artifacts: https://github.com/nmeum/android-tools/actions/runs/5231717758 https://github.com/nmeum/android-tools/suites/13513260172/artifacts/742693850

I'm still testing the more recent protobuf-devel 22.5 with the arch patch.

Build with /usr/lib64/libprotobuf.so (found version "4.22.5") also works (protobuf-devel 22.5).

munix9 commented 1 year ago

If I tried to build the latest master branch I get the following linking error in ArchLinux. I am not sure if it is related to protobuf change. Any idea?

/usr/bin/ld: vendor/libliblpdump.a(dynamic_partitions_device_info.pb.cc.o): undefined reference to symbol '_ZN4absl12lts_2023012512log_internal21CheckOpMessageBuilder7ForVar2Ev'
/usr/bin/ld: /usr/lib/libabsl_log_internal_check_op.so.2301.0.0: error adding symbols: DSO missing from command line

Strange, because the github workflow ran without errors, even for arch.

Biswa96 commented 1 year ago

Please see the linked pull request. The same linking error happens with opensuse ~leap~ tumbleweed, archlinux and macos also. The abseil-cpp library may have some issue.

munix9 commented 1 year ago

But the build / linux (opensuse/leap:15.4, clang gcc11-c++, CC=clang CXX=clang++) worked, didn't it?

I think the reason it fails on the others with a newer protobuf is the missing part of the patch:

--- android-tools-34.0.1.orig/vendor/CMakeLists.txt     2023-06-28 23:15:15.544237905 +0200
+++ android-tools-34.0.1/vendor/CMakeLists.txt  2023-06-28 23:45:28.672277935 +0200
@@ -73,6 +73,8 @@
 pkg_check_modules(libzstd REQUIRED IMPORTED_TARGET libzstd)

+find_package(Protobuf CONFIG REQUIRED)
 find_package(Protobuf REQUIRED)
+set(PROTOBUF_LIBRARIES protobuf::libprotobuf)
 set(THREADS_PREFER_PTHREAD_FLAG ON)
 find_package(Threads REQUIRED)

But that would still have to be tested, for me locally it worked without error anyway.

The problem is that with the missing part of the patch, the build with older protobuf does not work because there find_package(Protobuf CONFIG REQUIRED) fails (and possibly more).

A query of the protobuf version in cmake would be helpful to use the correct configuration - or something similar.

munix9 commented 1 year ago

Ok, with this change it should work:

--- android-tools-34.0.1.orig/vendor/CMakeLists.txt     2023-06-28 23:15:15.544237905 +0200
+++ android-tools-34.0.1/vendor/CMakeLists.txt  2023-06-28 23:45:28.672277935 +0200
@@ -73,6 +73,8 @@
 pkg_check_modules(libzstd REQUIRED IMPORTED_TARGET libzstd)

+find_package(Protobuf CONFIG)
 find_package(Protobuf REQUIRED)
+set(PROTOBUF_LIBRARIES protobuf::libprotobuf)
 set(THREADS_PREFER_PTHREAD_FLAG ON)
 find_package(Threads REQUIRED)

But it's more of a hack and not quite cleanly solved, but locally it works for me with old and new protobuf.

Biswa96 commented 1 year ago

I have added the workaround to fix the linking issue. Please review the second commit.

A query of the protobuf version in cmake would be helpful to use the correct configuration - or something similar.

Adding a version check may not work. It seems that cmake recently fixed the issue with newer protobuf version numbers https://gitlab.kitware.com/cmake/cmake/-/commit/fc7dcc6a24ffd33b780104ebd9dbb115d306827e

set(PROTOBUF_LIBRARIES protobuf::libprotobuf)

This sets the PROTOBUF_LIBRARIES variable which is already defined by cmake. So, I replaced PROTOBUF_LIBRARIES with protobuf::libprotobuf in all cases because the later is defined for all cmake and protobuf versions.