Repository navigation
Support add_subdirectory and ExternalProject_Add in third-party projects, improve CI - #565
Conversation
Use CMAKE_CURRENT_SOURCE_DIR instead of CMAKE_SOURCE_DIR to correctly locate package.json when metagraph is used as a submodule via add_subdirectory()
…per include propagation
…include propagation - Moved metagraph-core target definition early (before includes) - Converted all include_directories() to target_include_directories(metagraph-core PUBLIC ...) - Used GLOB_RECURSE for cleaner file globbing - Moved findFolly() section to after target definition for better organization - macOS system includes now added to metagraph-core target - All includes now properly propagate to dependent targets
…or folly-config.h generation This fixes the issue where folly-config.h was generated in the wrong location when metagraph is used as a submodule.
…lobal add_definitions with METAGRAPH_ALPHABET_DEF variable\n- Apply target_compile_definitions(metagraph-core PUBLIC )\n- Ensures external dependents (examples) receive the macro
…d_definitions with target_compile_definitions; propagate alphabet define via METAGRAPH_PUBLIC_DEFS; remove global link_directories; fix folly config include path for submodule builds
When metagraph is used as a subdirectory, CMAKE_CXX_FLAGS changes don't propagate. Add OpenMP_CXX_FLAGS directly to metagraph-core target with PUBLIC visibility so consumers inherit -fopenmp and get automatic OpenMP library linking from GCC.
GCC requires -fopenmp during linking to automatically add -lgomp. Previously only added it to compile options, but link stage also needs it.
SDSL is now built automatically via CMake ExternalProject, so manual installation with install.sh is no longer needed. Changes: - Removed 'install sdsl-lite' step from Linux CI workflow - Removed 'install sdsl-lite' step from MacOS CI workflow - Removed sdsl-lite installation instructions from documentation
Tests that the examples repository builds and runs correctly: - Checks out ratschlab/metagraph-examples with submodules - Builds the examples using CMake - Runs basic_query example with test data This ensures the examples stay compatible with MetaGraph changes.
- Fetches and checks out current metagraph commit in the examples submodule - Tests examples against the PR/push being evaluated - Removed custom path to use default checkout location
Test metagraph-examples on both Ubuntu and MacOS: - Ubuntu 22.04 with g++-13 - MacOS latest (Apple Silicon) - Platform-specific dependency installation
…udata, as it is needed for boost.
| #include <optional> | ||
| #include <vector> | ||
|
|
||
| #include "common/config.hpp" |
There was a problem hiding this comment.
We use some compile definitions, in this particular file MODE_TI, which are used by sdsl. If Metagraph is added via ExternalProject_Add, the parent project must also provide them as defines (which is done by generating them into common/config.hpp), otherwise there will be linking errors.
There was a problem hiding this comment.
So the parent project doesn't have to define those flags? But how can we be sure not to miss these flags in some files? Or, are we going to add this common/config.hpp into every file in the project?
There was a problem hiding this comment.
Yes, parent projects need to provide them when using ExternalProject_Add or make install.
When using add_subdirectory, parent project inherits them automatically, but with ExternalProject_Add or system-wide install via make install, it's tricky, and could be argued that parent project shouldn't be responsible for matching all of them when using library. For example, it wouldn't be sufficient to define CMAKE_DBG_ALPHABET like it's done in add_subdirectory, as one would also need to provide internal definitions like _DNA_GRAPH.
When choosing places to add common/config.hpp, I added it anywhere the file or its external header dependency relies on these definitions. As it's now in CI, we'll be notified if missing a definition straight up leads to compilation or linking error, or if it prevents even basic usage of Metagraph.
In any case, it will not affect directly building Metagraph from sources or using it via add_subdirectory because we still keep compile definitions in this case. If we're not satisfied it covers all use-cases for ExternalProject, we can also recommend parent projects to use -include common/config.hpp compilation flag when in doubt, to force-include it? But I'm not sure there's much more we can do on our end to ensure proper propagation, short of adding it to all files, which I don't think we want to do.
There was a problem hiding this comment.
I think it's fine to leave this to the user, if we specify which flags need to be set. Similar how we add MODE_TI when we use sdsl.
IMO The more config.hpp files we add, the more potential freedom for bugs we create
There was a problem hiding this comment.
I suppose another option for us would be to remove include "commom/config.hpp" from our files and to tell external projects that they need to include it before any other of our headers? Not sure what would be the best practice here.
There was a problem hiding this comment.
Yes, I think that stuff can be inside metagraph.hpp
https://github.com/ratschlab/metagraph-examples/blob/master/examples/basic_query.cpp#L29C1-L29C25
| // Core graph and annotation structures | ||
| #include "graph/annotated_dbg.hpp" | ||
|
|
||
| // Configuration management |
There was a problem hiding this comment.
This comment could be more informative. As now, it's not helpful at all
| // Auto-generated compile definitions for MetaGraph | ||
| // This file is auto-generated by CMake and should not be edited manually | ||
| // Note: These definitions complement (not replace) CMake compile definitions |
There was a problem hiding this comment.
How should this be edited if not manually?
There was a problem hiding this comment.
The config.hpp.in should be edited manually. The comment refers to the config.hpp file that CMake generates based on it, as .in file is a template for it.
There was a problem hiding this comment.
Not ideal. First, because unclear what the comment refers to. Second, because there is another config.hpp in the project (cli/config/config.hpp). Last, the things we discussed in DM
There was a problem hiding this comment.
Will clarify. Can also rename file to compile_definitions.hpp or similar.
| #include <sdsl/uint128_t.hpp> | ||
| #include <sdsl/uint256_t.hpp> | ||
|
|
||
| #include "common/config.hpp" |
There was a problem hiding this comment.
Shouldn't it go before <sdsl/uint128_t.hpp>? That uses MODE_TI
Makes MetaGraph easy to integrate into external projects using CMake's
add_subdirectory()orExternalProject_Add.metagraph/api/metagraph.hppconvenience headerccachefor faster builds