Conversation
John-LittleBearLabs
left a comment
John-LittleBearLabs
left a comment
There was a problem hiding this comment.
I guess the TL;DR is: how are you managing your protobuf dependency?
I'll make a point to take it into consideration while cleaning up the current mess.
| @@ -74,7 +73,7 @@ target_include_directories(ic_proto | |||
| ) | |||
| target_link_libraries(ic_proto | |||
There was a problem hiding this comment.
First of all, thanks for your PR!
Secondly, I want to apologize. This codebase is in a transition and there's no way you could've known that. The logical "head" of this target is actually over here at least for now while waiting on an answer about something else...
If I can get my head around your use case and reproduce the issue then I'll move a version of your fix into the place I'll need things to be as I clean up my mess.
There was a problem hiding this comment.
my use case was just getting the following to work on ubuntu noble LTS:
cmake -D CMAKE_BUILD_TYPE=Release -D DOWNLOAD_CHROMIUM=TRUE -S ~/ipfs-chromium -B ~/build
There was a problem hiding this comment.
cmake --version
cmake version 3.28.3
python3 --version
Python 3.12.3
ldd --version
ldd (Ubuntu GLIBC 2.39-0ubuntu8.4) 2.39
There was a problem hiding this comment.
I appreciate the info. Your versions are reasonably close to my own, well within what I intended to support. And that cmake command was definitely intended to work.
| find_package(nlohmann_json QUIET REQUIRED CONFIG) | ||
| find_package( | ||
| Protobuf | ||
| ${protobuf_version} |
There was a problem hiding this comment.
This would break my workflow - it would allow CMake to find system-installed or Chromium-installed protobuf.
The most important thing is of course that your libprotobuf version matches your protoc version exactly.
Should I assume you're trying not to use conan?
Would it be helpful if we had the version to be passed into the CMake part of the build (which is typically run by conan, but shouldn't have to be) as a variable like cmake -D PROTOBUF_VERSION=5.27.0 ?
There was a problem hiding this comment.
i'm using conan. removing the version check might have been unnecessary, the blocker for me was the fix below on line 76
There was a problem hiding this comment.
and prob because im using the dev version of protobuf
protoc --version
libprotoc 31.0-dev
There was a problem hiding this comment.
Thanks for pointing it out. I think "libprotobuf" is just more correct regardless, but I'll check to make sure it works for me.
Is it OK if I leave this PR open here for a while? You can use your fork for now and after I get my stuff together I'll let you know and we'll see if things work OK for you... sound good?
There was a problem hiding this comment.
yeah let's leave it open as things shake out
| target_link_libraries(ic_proto | ||
| PUBLIC | ||
| protobuf::protobuf | ||
| protobuf::libprotobuf |
There was a problem hiding this comment.
2 participants
No description provided.