apache / incubator-graphar

An open source, standard data file format for graph data storage and retrieval.
https://graphar.apache.org/
Apache License 2.0
217 stars 46 forks source link

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

Open acezen opened 2 weeks ago

acezen commented 2 weeks ago

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 commented 1 week ago

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

jasinliu commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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).