Project modernization #79
Conversation
cmake create the metis.h when generate.
LecrisUT
force-pushed
the
cmake/modernization
branch
2 times, most recently
from
dcd9efc to
2050292
Compare
|
I don't want to sound overly pessimistic, but what gives you the confidence that Prof. Karypis will merge your code? There are trivial pull requests waiting for months to get any feedback. Also simple things like creating a tag for GKlib to create initial packages are not happening. |
This is due to the octopus project being keen on using METIS as a hard-dependency. I need to make these cahnges to be able to import it there anyway so I will be maintaining my fork that other people can use as well. This MR will be here for when Prof. Karypis wishes to make these changes official. |
LecrisUT
force-pushed
the
cmake/modernization
branch
from
1cb8be1 to
33d2e6b
Compare
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
LecrisUT
force-pushed
the
cmake/modernization
branch
from
33d2e6b to
7779366
Compare
This resolves the compatibility with `set_package_properties()` Signed-off-by: Cristian Le <[email protected]>
|
@gruenich If you find it useful feel free to give it a try. Right now the project is importable, and I will test on octopus if it is usable in this form. The tests I couldn't get them to work properly, but with some feedback we can see this through. |
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
| target_link_libraries(METIS_METIS PRIVATE GKlib::GKlib) | ||
|
|
||
| if (TARGET GKlib_GKlib) | ||
| target_compile_options(GKlib_GKlib PRIVATE -fPIC) |
There was a problem hiding this comment.
Indeed, it is marked as a temp commit because I am not sure how to properly handle it. Should GKlib become a shared library and be installed together with METIS, or remain a static, but patch it to be PIC. This is the first time I've encountered where this has become an issue so I'm not 100% sure of the proper handling. I'm leaning towards the latter.
| LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR} COMPONENT METIS_Runtime NAMELINK_COMPONENT METIS_Development | ||
| ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR} COMPONENT METIS_Development | ||
| PUBLIC_HEADER DESTINATION ${CMAKE_INSTALL_INCLUDEDIR} COMPONENT METIS_Development | ||
| RUNTIME DESTINATION ${CMAKE_INSTALL_RUNTIMEDIR} COMPONENT METIS_Runtime |
There was a problem hiding this comment.
Good catch, not sure where that came from when copying from my other projects
|
Is this repo still actively maintained by Prof. Karypis? Hope this PR will be merged soon. |
1 similar comment
|
Is this repo still actively maintained by Prof. Karypis? Hope this PR will be merged soon. |
| GIT_TAG cmake/modernization | ||
| FIND_PACKAGE_ARGS ${gklib_find_package_args} | ||
| ) | ||
| METIS_FetchContent_MakeAvailable(GKlib) |
There was a problem hiding this comment.
While this is correct in principle, it excludes the possibility of using a pre-built gklib via cmake, which you spent effort on in your other PR. So I think something like this would be better:
find_package(GKlib)
if (NOT TARGET GKlib::GKlib)
# ......
# FetchContent_Declare(....
endif()I modified this locally and find_package(GKlib) works great with your other CMake.
| include(FetchContent) | ||
| if (METIS_INSTALL) | ||
| include(CMakePackageConfigHelpers) | ||
| if (UNIX) |
There was a problem hiding this comment.
This should be unconditional, as you use e.g. CMAKE_INSTALL_BINDIR elsewhere. It's not a problem to use GNUInstallDirs on Windows.
| # Configure main target to consume the metis.h | ||
| # TODO: Move to FILE_SET for cmake 3.23 | ||
| set_target_properties(METIS_METIS PROPERTIES | ||
| PUBLIC_HEADER ${CMAKE_CURRENT_SOURCE_DIR}/metis.h |
There was a problem hiding this comment.
This should be CMAKE_CURRENT_BINARY_DIR
Coming in are a bunch of project modernization. I will make a summary of all of the changes later on. Requires an equivalent change in GKlib, coming soon
Pinging @shengjianda as they initiated this movement
Closes #10, #18
Depends on: KarypisLab/GKlib#33