Skip to content
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.

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

fix(c++): fix link error -lgar_arrow_static not found when link static graphar build arrow from source #628

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

Conversation

acezen
Copy link
Contributor

@acezen acezen commented Sep 12, 2024

Reason for this PR

as issue #627 describes, the gar_arrow_static symbol is not valid when the graphar building is done.
consider to link to the $GAR_ARROW_STATIC_LIB, instead gar_arrow_static

What changes are included in this PR?

merge related bundled builded arrow static libraries into a single libraries 'libgraphar_bundle_dependencies.a'

Are these changes tested?

yes

Are there any user-facing changes?

no

@acezen
Copy link
Contributor Author

acezen commented Sep 18, 2024

hi, @jasinliu the PR is ready and work as expected, can yo help to review it?

@jasinliu
Copy link
Contributor

There is a problem. After installing graphar in the system using this method, if you then install arrow in the system and then rely on both arrow and graphar in a program, the compilation will throw an error about multiple definitions.

install graphar

cmake .. -DBUILD_ARROW_FROM_SOURCE=ON -DGRAPHAR_BUILD_STATIC=ON
make -j && sudo make install

install arrow

cmake ..  -DARROW_PARQUET=ON -DARROW_CSV=ON -DARROW_JSON=ON  -DARROW_ORC=ON -DARROW_DATASET=ON -DARROW_BUILD_SHARED=OFF -DARROW_DEPENDENCY_USE_SHARED=OFF
sudo make install

show.cc

#include "graphar/graph_info.h"
#include "iostream"
#include "arrow/api.h"


int main() {
    std::string path = "/workspaces/incubator-graphar/testing/neo4j/MovieGraph.graph.yml";
    auto graph_info = graphar::GraphInfo::Load(path);
    std::cout << graph_info.value()->Dump().value() << std::endl;
    arrow::Int8Builder int8builder;
}

CMakeLists.txt

cmake_minimum_required(VERSION 3.15)

project(show)

set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++17 -Wall")

set(TARGET show)

find_package(OpenSSL REQUIRED)
find_package(graphar REQUIRED)
find_package(Arrow REQUIRED)

add_executable(${TARGET} show.cc)
target_link_libraries(${TARGET} PRIVATE graphar Arrow::arrow_static)

build show

cmake .. && make

image

@yecol
Copy link
Contributor

yecol commented Sep 18, 2024

Hi @jasinliu

Seems it works as expected:

The problem comes from your CMakeList.txt:
you are linking a static library libgraphar.a, which bundled libarrow.a; and Arrow::arrow_static again.

How about a revised CMakeList.txt like this:

cmake_minimum_required(VERSION 3.15)

project(show)

set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++17 -Wall")

set(TARGET show)

find_package(OpenSSL REQUIRED)
find_package(graphar REQUIRED)
# find_package(Arrow REQUIRED)

add_executable(${TARGET} show.cc)
target_link_libraries(${TARGET} PRIVATE graphar)

@jasinliu
Copy link
Contributor

Hi @jasinliu

Seems it works as expected:

The problem comes from your CMakeList.txt: you are linking a static library libgraphar.a, which bundled libarrow.a; and Arrow::arrow_static again.

How about a revised CMakeList.txt like this:

cmake_minimum_required(VERSION 3.15)

project(show)

set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++17 -Wall")

set(TARGET show)

find_package(OpenSSL REQUIRED)
find_package(graphar REQUIRED)
# find_package(Arrow REQUIRED)

add_executable(${TARGET} show.cc)
target_link_libraries(${TARGET} PRIVATE graphar)

Thanks for the reminder.

But I also need arrow header in system to enable #include <arrow/api.h>. Although I can install arrow separately to use the header file, this may cause the header file of arrow(in system) to be inconsistent with the actual implementation(libgraphar_bundled_dependencies.a).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants