Skip to content

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)
Copy link

@augusew1 augusew1 May 28, 2024

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`
# 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.

@karypis
Copy link
Contributor

karypis commented Jul 4, 2025

I merged the development branch in master that have addressed some of those. If you have time and update the PR to reflect those changes, I'll be able to take a look at it this summer.

@karypis
Copy link
Contributor

karypis commented Jul 5, 2025

Not sure how many of these still apply given the recent changes.

@augusew1
Copy link

augusew1 commented Jul 5, 2025

I can attempt to pull in the latest and see if the changes still work. There is the related KarypisLab/METIS#79 branch that would really help out with Windows builds for METIS.

@augusew1
Copy link

augusew1 commented Jul 6, 2025

I was able to rebase and I would say the vast majority of changes are still necessary. As for the content of the code, I think there are some cmake improvements that could be made. Some of it is style choice, some of it is cmake best practices. Perhaps I could work with @LecrisUT on this, if they are willing.

@LecrisUT
Copy link
Author

LecrisUT commented Jul 6, 2025

Sure, I can give a re-review. I of course picked up a lot of design practices since this PR and there are aome minor details that I would change. The overall re-design is still there in order to make this be usable by other projects.

The only issue is if the maintainer is welcoming to these, then we can start suggesting piece-wise improvements. I can make a breakdown of the main changes that I would recommend and then address them in individual PRs

@augusew1
Copy link

augusew1 commented Jul 7, 2025

I think the interest is renewed, and if you rebase your fork locally and this branch, I can make some PRs toward it.

Piecewise improvements are likely not going to be possible as we're effectively re-writing the build system.

@LecrisUT
Copy link
Author

LecrisUT commented Jul 8, 2025

Looking at the changes made, I believe we can still do piecewise changes, e.g. chainging the CI, adding higher bound policy, etc. We should start over this one imo. I really urge @karypis to comment on the future plans for this project and maintenance. I would start with an issue reviewing the current state of master on what changes I would recommend (and the whys if requested)

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.

4 participants