protobuf: Bump version/add depends on abseil-cpp#25566
protobuf: Bump version/add depends on abseil-cpp#25566vidplace7 wants to merge 1 commit intoopenwrt:masterfrom
Conversation
vidplace7
force-pushed
the
protobuf_update
branch
8 times, most recently
from
7c1321d to
385334f
Compare
Found this message deep in the protobuf commits. @BKPepe seems like Python bindings should be built from PyPI sources. I will remove the Python bindings from this change for now. I've not seen examples of other packages that bundle a cmake package an a pypi package in the same Makefile. I'd imagine the python bindings will need to be split up into it's own package? |
vidplace7
force-pushed
the
protobuf_update
branch
from
385334f to
e3f4d58
Compare
vidplace7
changed the title
vidplace7
force-pushed
the
protobuf_update
branch
3 times, most recently
from
a9eec0a to
4cbf179
Compare
|
How you would overcome the Node compilation issue with abesil-cpp. [EDIT] Linking error fixed by : The issue is with CMAKE_CXX_STANDARD, taken from Microsoft vcpkg. [EDIT3] |
This seems like a wise move, so that protobuf-c isn't broken. Thanks for linking that discussion!
I drew inspiration from OpenEmbedded and buildroot for the Wouldn't setting the CXXSTD to 17 preclude any targets that don't support C++17 (but do support 14)? Seems Microsoft is addressing that concern with some conditional logic, from your example. set(ABSL_USE_CXX17_OPTION "")
if("cxx17" IN_LIST FEATURES)
set(ABSL_USE_CXX17_OPTION "-DCMAKE_CXX_STANDARD=17")
endif()A similar approach may be warranted here. |
|
Sorry about my previous info, protobuf-c seems compile with protobuf 29.1, but I am not sure either package which are depend on protobuf and protobuf-c will also compile. For example netdata.
Yes, nodejs is tough one to fix. My temp workaround is move back with version 20.18.1 LTS which doesn't use abseil-cpp at all.
In my build environment (glibc, targeting x86_64), compilation will still succeed without : The question is, is it default into 17 or 14. More likely or probably into 17 by default as with 14 I am getting those linking errors. |
vidplace7
force-pushed
the
protobuf_update
branch
from
4cbf179 to
231bf6c
Compare
|
Really weird, now node (version 22.12.0 LTS) doesn't segmentation fault anymore when compiling node-yarn!. Patch for node still needed for compilation. Is there any reason on why you don't use absl_DIR? [EDIT] |
| +# PROPERTY INSTALL_RPATH "$ORIGIN/../${CMAKE_INSTALL_LIBDIR}") | ||
| elseif (APPLE) | ||
| set_property(TARGET ${binary} | ||
| PROPERTY INSTALL_RPATH "@loader_path/../lib") |
There was a problem hiding this comment.
can this patch be cleaned up?
or upstreamed?
maybe it's fine, but i'm a bit lazy (right now) to try to understand it :)
There was a problem hiding this comment.
I added that. IIRC it causes linking to OS libraries instead of OpenWrt ones.
There was a problem hiding this comment.
Actually, as of d24229e this should not be needed.
|
This is not compiling on my side with the folloing error: |
|
You could try my finding above. |
|
no dice @vortexilation, the error states: EDIT Nvm it works, now I'm battling another error: |
|
@tiagogaspar8 |
|
Damn, you always know how to fix stuff @vortexilation ahahah I can now compile everything and I tested netdata's claim and it works, I'll update my PR, yet it needs abseil and protobuf fixed, curious to see the outcome of this PR. |
|
@tiagogaspar8 protobuf-c need a PR also, perhaps someone might wanted to open a PR?. Based on gentoo's commit from here , libprotobuf-c also needs this patch, Tested with compiling frr package with this PR + my changes. |
|
I think the next steps would be:
Now, regarding Abseil, is the fix you provided the correct fix for this issue @vortexilation? |
|
Good idea about the steps. I am sure for libprotobuf-c is the correct one, even though it's from test branch. I am not sure if using test branch is allowed in OpenWrt ?. Also there are no other choice other than using test branch. Perhaps @neheb & @vidplace7 may comment on my proposed fixes. Also please be warned, mostly I am only compile tested not run-tested or testing thoroughly the functionalities of aforementioned compilation result. |
|
Hi @vidplace7 |
vidplace7
force-pushed
the
protobuf_update
branch
2 times, most recently
from
1ec43e5 to
d5aae19
Compare
vidplace7
force-pushed
the
protobuf_update
branch
2 times, most recently
from
10d9dc0 to
ea0041d
Compare
vidplace7
force-pushed
the
protobuf_update
branch
2 times, most recently
from
3f2e670 to
fb7f284
Compare
|
I have incorporated fixes from @vortexilation and removed the rpath patch per @neheb. |
vidplace7
force-pushed
the
protobuf_update
branch
from
fb7f284 to
420180a
Compare
vidplace7
force-pushed
the
protobuf_update
branch
3 times, most recently
from
297fa2f to
6856f49
Compare
vidplace7
force-pushed
the
protobuf_update
branch
from
6856f49 to
fe35ea7
Compare
|
@neheb Are you available to review this PR? 👍 I think all the loose ends have been tied up here, and I'd love to stop managing this out of tree in meshtastic/openwrt (where it's been working well for nearly a year) |
This comment was marked as spam.
This comment was marked as spam.
vidplace7
force-pushed
the
protobuf_update
branch
2 times, most recently
from
207d843 to
572950c
Compare
* Bump protobuf to v29.5 * Add dependency on abseil-cpp, required by protobuf v22+ Signed-off-by: Austin Lane <vidplace7@gmail.com>
vidplace7
force-pushed
the
protobuf_update
branch
from
572950c to
7f750cf
Compare
|
@neheb is anything else blocking this? This seem to be blocking Meshtastic upstreaming. |
This comment was marked as outdated.
This comment was marked as outdated.
|
All 4 projects - mosh, ola, pdns and spoofer - that depend on protobuf fail to compile with this patchset. I haven't looked too deep into it, so not sure if directly related or not. I think this needs to be addressed first. |
| TITLE:=A structured data encoding library | ||
| URL:=https://github.com/google/protobuf | ||
| DEPENDS:=+zlib +libpthread +libatomic +libstdcpp | ||
| DEPENDS:=+zlib +libpthread +libatomic +libstdcpp +abseil-cpp |
| if (UNIX AND NOT APPLE) | ||
| - set_property(TARGET ${_library} | ||
| - PROPERTY INSTALL_RPATH "$ORIGIN") | ||
| +# set_property(TARGET ${_library} |
7 participants
Maintainer: @neheb
Compile tested:
Description:
Update
protobufand add Python bindings (this can be split into another PR if necessary).Note that
protobufversions >= v22 requireabseil-cpp.https://protobuf.dev/support/migration/#abseil
Depends on:
Marking as draft for now, pendingabseil-cpp#25802 review.