Project modernization #79

LecrisUT · 2023-10-26T15:17:48Z

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

cmake create the metis.h when generate.

gruenich · 2023-10-26T21:07:08Z

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.
Maybe wait until you get some reactions or things merged, before you invest too much time.

LecrisUT · 2023-10-26T21:25:35Z

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.

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.

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

This resolves the compatibility with `set_package_properties()` Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

LecrisUT · 2023-10-27T15:28:41Z

@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 <cristian.le@mpsd.mpg.de>

xantares · 2023-11-10T19:12:34Z

src/libmetis/CMakeLists.txt

+target_link_libraries(METIS_METIS PRIVATE GKlib::GKlib)
+
+if (TARGET GKlib_GKlib)
+	target_compile_options(GKlib_GKlib PRIVATE -fPIC)


use POSITION_INDEPENDENT_CODE

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.

xantares · 2023-11-10T19:13:18Z

src/libmetis/CMakeLists.txt

+			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


CMAKE_INSTALL_BINDIR

Good catch, not sure where that came from when copying from my other projects

adam-sim-dev · 2023-11-30T13:11:46Z

Is this repo still actively maintained by Prof. Karypis? Hope this PR will be merged soon.

adam-sim-dev · 2023-11-30T13:12:14Z

Is this repo still actively maintained by Prof. Karypis? Hope this PR will be merged soon.

augusew1

Once again, thank you for doing this! I am testing on Windows and found these bugs. Otherwise, this works great and I'm able to build the project quite easily.

augusew1 · 2024-05-28T22:06:19Z

CMakeLists.txt

+		GIT_TAG cmake/modernization
+		FIND_PACKAGE_ARGS ${gklib_find_package_args}
+)
+METIS_FetchContent_MakeAvailable(GKlib)


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.

@LecrisUT Could you please address these comments by @augusew1 so we can use your lastest branch?

Sure, I can revisit this and see what I need to update. I have gained quite a few insight in CMake design since writing this and I would want to simplify this quite a bit.

But first, I would suggest considering where to host ongoing maintenance, since this repo is dead. I do not have the domain knowledge to maintain this, just the build-system knowledge to assist. @scivision also provides a variant that is used in downstream https://github.com/scivision/METIS, but I don't know if that isn't a similar story as well.

If there are interested maintainers, I would suggest making an organization, and soft-forking the project and continue there officially.

Sure, I can revisit this and see what I need to update. I have gained quite a few insight in CMake design since writing this and I would want to simplify this quite a bit.

But first, I would suggest considering where to host ongoing maintenance, since this repo is dead. I do not have the domain knowledge to maintain this, just the build-system knowledge to assist. @scivision also provides a variant that is used in downstream https://github.com/scivision/METIS, but I don't know if that isn't a similar story as well.

If there are interested maintainers, I would suggest making an organization, and soft-forking the project and continue there officially.

scivision's fork of METIS does not work as expected at the time I tested it. See the issue I opened there. I use his CMake repository for MUMPS, which is actively updated.

Weird looking at the implemenation, I see that we have the same approach in
scivision/METIS@ae95e7a#diff-1fa6d22cb50f611cea6c06a4523dc8602907d4b30b6da0d05b6cce52fb18a546

They have added it as -Dintsize/-Drealsize, but it was not added as a proper cache variable in
https://github.com/scivision/METIS/blob/7f8f9b0d765c15efe3eefe3e9db0a9faf9a5f480/CMakeLists.txt#L16-L21
Probably they have overlooked that issue.

I suspect that a more proper solution is to redefine that variable since some other users seem to manually #define that macro in order to get it to work which clashes with the configured file #define. But these design issues are the things where a proper maintainer is preferred to actually state what is the usage and what fix is preferred.

It looks like there is renewed interest in updating the bulid system. So it would be nice to clean up this PR and make sure it works on Windows as well as Linux. Additionally, while I wouldn't say I'm the biggest fan of cmake overall, I do see some improvements that could be made to the branch overall. I'm more than willing to collaborate on that.

augusew1 · 2024-05-28T22:07:21Z

CMakeLists.txt

+include(FetchContent)
+if (METIS_INSTALL)
+	include(CMakePackageConfigHelpers)
+	if (UNIX)


This should be unconditional, as you use e.g. CMAKE_INSTALL_BINDIR elsewhere. It's not a problem to use GNUInstallDirs on Windows.

augusew1 · 2024-05-28T22:08:35Z

include/CMakeLists.txt

+# 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


This should be CMAKE_CURRENT_BINARY_DIR

shengjianda and others added 6 commits August 10, 2023 01:11

metis can be used as submodul, with gklib is also submodule.

db9e40d

cmake create the metis.h when generate.

fix config bug

ee28bbe

Add basic section structure

879c8c2

Bump cmake_minimum_required to something useful

7c69bfe

Rearrange options and implement some defaults

f1b40ba

Add C standard specification

e7af6aa

LecrisUT force-pushed the cmake/modernization branch 2 times, most recently from dcd9efc to 2050292 Compare October 26, 2023 15:40

LecrisUT mentioned this pull request Oct 27, 2023

Project modernization KarypisLab/GKlib#33

Open

LecrisUT force-pushed the cmake/modernization branch from 1cb8be1 to 33d2e6b Compare October 27, 2023 10:49

LecrisUT added 11 commits October 27, 2023 15:13

CMake project modernization

0855b14

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

Add basic CMakePresets

6933180

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

Add basic GitHub actions

819421a

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

Import GKlib from FetchContent

7993b08

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

[Temp] Link to fork project

3d1068b

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

[Temp] Add -fPIC flag to imported GKlib

21ce25b

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

Refactor testsuite

47c8e6b

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

Refactor project location to intuitive location

2aab489

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

Use standard OpenMP macros

75c14c3

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

Add OpenMP option

d86ab47

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

Add OpenMP GitHub action

40fa369

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

Disable tests. They are completely broken

7779366

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

LecrisUT force-pushed the cmake/modernization branch from 33d2e6b to 7779366 Compare October 27, 2023 14:31

Add FetchContent wrapper

d8721f0

This resolves the compatibility with `set_package_properties()` Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

LecrisUT marked this pull request as ready for review October 27, 2023 15:25

LecrisUT added 2 commits October 27, 2023 17:30

[Temp] Mark some environments as experimental

d5cd358

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

Remove outdated files and references

c2af072

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

xantares reviewed Nov 10, 2023

View reviewed changes

augusew1 reviewed May 28, 2024

View reviewed changes

Project modernization #79

Are you sure you want to change the base?

Project modernization #79

Uh oh!

Conversation

LecrisUT commented Oct 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gruenich commented Oct 26, 2023

Uh oh!

LecrisUT commented Oct 26, 2023

Uh oh!

LecrisUT commented Oct 27, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adam-sim-dev commented Nov 30, 2023

Uh oh!

adam-sim-dev commented Nov 30, 2023

Uh oh!

augusew1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

LecrisUT commented Oct 26, 2023 •

edited

Loading