Skip to content

libprotobuf cmake fix #132

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
maceip wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

libprotobuf cmake fix #132

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions library/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ project(ipfs_client
)
set(old_path ${CMAKE_MODULE_PATH})
set(CMAKE_MODULE_PATH ${CMAKE_CURRENT_LIST_DIR};${CMAKE_CURRENT_BINARY_DIR})

include(FindProtobuf)
if(IN_WORKSPACE)
include(../cmake/setup.cmake)
list(APPEND CMAKE_MODULE_PATH ${old_path})
Expand All @@ -17,7 +17,6 @@ else()
find_package(nlohmann_json QUIET REQUIRED CONFIG)
find_package(
Protobuf
${protobuf_version}
Copy link
Collaborator

@John-LittleBearLabs John-LittleBearLabs Feb 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Author

@maceip maceip Feb 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm using conan. removing the version check might have been unnecessary, the blocker for me was the fix below on line 76

Copy link
Author

@maceip maceip Feb 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and prob because im using the dev version of protobuf

protoc --version
libprotoc 31.0-dev

Copy link
Collaborator

@John-LittleBearLabs John-LittleBearLabs Feb 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

@maceip maceip Feb 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah let's leave it open as things shake out

QUIET
REQUIRED
CONFIG
Expand Down Expand Up @@ -74,7 +73,7 @@ target_include_directories(ic_proto
)
target_link_libraries(ic_proto
Copy link
Collaborator

@John-LittleBearLabs John-LittleBearLabs Feb 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@maceip maceip Feb 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@maceip maceip Feb 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

cmake --version
cmake version 3.28.3

python3 --version
Python 3.12.3

ldd --version
ldd (Ubuntu GLIBC 2.39-0ubuntu8.4) 2.39

Copy link
Collaborator

@John-LittleBearLabs John-LittleBearLabs Feb 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

PUBLIC
protobuf::protobuf
protobuf::libprotobuf
Copy link
Collaborator

@John-LittleBearLabs John-LittleBearLabs Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://gitlab.com/jbt/ipfs_client/-/merge_requests/3#note_2429365029

)

add_library(ipfs_client
Expand Down