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

Project modernization #33

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

LecrisUT
Copy link

This is the counterpart of KarypisLab/METIS#79

@LecrisUT LecrisUT force-pushed the cmake/modernization branch from e3b23da to 3379e9f Compare October 27, 2023 10:45
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 LecrisUT force-pushed the cmake/modernization branch from 3379e9f to 1ba74b3 Compare October 27, 2023 14:30
@LecrisUT LecrisUT marked this pull request as ready for review October 27, 2023 15:25
Copy link

@augusew1 augusew1 left a comment

Choose a reason for hiding this comment

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

Thank you for doing this, I am testing this on MSVC and clang on Windows and I ran into a couple of issues. My only other request would be that a .pc file would be nice, otherwise the cmake works just fine!

# win32/adapt.h
# )
if (GKLIB_INSTALL)
install(FILES win32/adapt.h ${CMAKE_INSTALL_INCLUDEDIR}/win32)

Choose a reason for hiding this comment

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

Typo here, should be

install(FILES win32/adapt.h DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/win32

add_feature_info(GKLIB_OpenMP GKLIB_OpenMP "OpenMP support")
option(GKLIB_PCRE "GKlib: Enable PCRE support" OFF)
add_feature_info(GKLIB_PCRE GKLIB_PCRE "PCRE support")
cmake_dependent_option(GKLIB_GKREGEX "GKlib: Enable GKREGEX support" OFF "NOT GKLIB_PCRE" OFF)

Choose a reason for hiding this comment

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

Logic error here, it should be

Need to add `-GKLIB_GKREGEX=ON`
```cmake
# Second "OFF" should be "ON"
cmake_dependent_option(GKLIB_GKREGEX "GKlib: Enable GKREGEX support" OFF "NOT GKLIB_PCRE" ON)

include(FetchContent)
if (GKLIB_INSTALL)
include(CMakePackageConfigHelpers)
if (UNIX)

Choose a reason for hiding this comment

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

This should be unconditional, it's not an issue for this on Windows. And also, you use e.g. CMAKE_INSTALL_BINDIR unconditionally later on which expands to nothing on Windows currently.

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