Cmake #487

LecrisUT · 2024-02-21T13:32:00Z

This PR contains both the CMake implementation and CI reorganization. We can split it up later, but lets keep them together until the CI pass

LecrisUT

@JeromeCCP9 Here's the re-implementation. I have left a few comments

LecrisUT · 2024-02-21T13:36:54Z

.github/workflows/step_test.yaml

+jobs:
+  tests:
+    name: Check on ${{ matrix.toolchain }} toolchain ${{ matrix.mpi }}
+    runs-on: ${{ matrix.os || 'ubuntu-latest' }}


There is no macos and windows here, because we need to discuss what compilers to use there and where to get them

LecrisUT · 2024-02-21T13:38:19Z

CMakeLists.txt

+		LANGUAGES Fortran C
+		)
+
+set(CMAKE_C_STANDARD 11)
+set(CMAKE_C_STANDARD_REQUIRED ON)
+set(CMAKE_C_EXTENSIONS OFF)


There are some new C sources, but header only? Probably we don't need to add C to the main project in that case.

Adding C is useful for compiling with MKL, without this cmake cannot detect MKL automatically
https://cmake.org/cmake/help/latest/module/FindBLAS.html#intel-mkl

It is a bit annoying since we are actually a fortran code, but without this users need to define a lot of cmake variables for MKL.

What about using find_package(MKL) by default if it's intel compiler, otherwise have an option WANNIER90_MKL option that uses either MKL or default BLAS/LAPACK? I am not sure if it works on a pure Fortran project, but we can try and see

Did a test, we can add some if conditionals to find either MKL or BLAS/LAPACK, however, in the

target_link_libraries(Wannier90_lib PRIVATE BLAS::BLAS LAPACK::LAPACK)

I guess we need to add another if for MKL as well, not sure if there is a simpler solution.

On the other hand, by enabling C language everything works automatically.

Ok, I suggest we postpone the decision of using either implementation, because @giovannipizzi warned me that there is an issue between LAPACK and macos, which we need to think how to handle.

LecrisUT · 2024-02-21T13:42:10Z

CMakeLists.txt

@@ -0,0 +1,179 @@
+cmake_minimum_required(VERSION 3.25...3.29)


I have upgraded the minimum CMake to be in sync with CentOS stream 9 (should be ported to RHEL 9, I'll try to make some noise on that) and Ubuntu 24.04. Users could use pip install cmake and HPCs should get ready.

LecrisUT · 2024-02-21T13:45:14Z

test-suite/CMakeLists.txt

+#[=============================================================================[
+#                                   Options                                   #
+]=============================================================================]
+
+#[=============================================================================[
+#                            Project configuration                            #
+]=============================================================================]


Placeholders for if we want to add coverage and sanitizer

test-suite/CMakeLists.txt

LecrisUT · 2024-02-21T17:56:25Z

It seems the only failing tests are with ifx, should we change it to ifort and try again, or mark it as experimental for now.

LecrisUT · 2024-02-22T09:17:41Z

test-suite/libv2-demo/lib-gas-opt-pllel/CMakeLists.txt

@@ -0,0 +1,7 @@
+cmake_minimum_required(VERSION 3.25...3.29)
+project(demo_Wannier90 LANGUAGES Fortran)


Here are functional tests for the library. Let me know what things to test in these cases.

LecrisUT · 2024-02-22T10:47:22Z

@JeromeCCP9 There are a few issues with the demo.F90 interface using different variable types. It is also a bit confusing how to deal with mpi_comm being integer in the module file if it's built without mpi. Other than that, the C/C++ interface seems to fail with: undefined reference to symbol 'CFI_establish@@GFORTRAN_9'. Could you assist me with these?

JeromeCCP9 · 2024-02-22T16:17:56Z

Yes, I'll attend to these, let me catch up this evening...

@JeromeCCP9 There are a few issues with the demo.F90 interface using different variable types. It is also a bit confusing how to deal with mpi_comm being integer in the module file if it's built without mpi. Other than that, the C/C++ interface seems to fail with: undefined reference to symbol 'CFI_establish@@GFORTRAN_9'. Could you assist me with these?

JeromeCCP9 · 2024-02-23T10:53:57Z

This last week I've made some changes to the library and provided a new test driver for the c++ interface in a branch "c-for-gc," and these will be merged shortly into develop. I think that the existent tests must be replaced (by me), so it would make more sense to remove the demo.F90 tests from this PR, if possible? If not, then I merge later, that's also ok.

(Also, at the moment, the c++ interface relies on a f2018 support (for CFI_establish) and this needs to be tested for.)

LecrisUT · 2024-02-23T11:36:34Z

This last week I've made some changes to the library and provided a new test driver for the c++ interface in a branch "c-for-gc," and these will be merged shortly into develop. I think that the existent tests must be replaced (by me)

I saw that, it's ok, just ping me and I will rebase and fix that.

remove the demo.F90

It would be good to have a Fortran test. In #444 I am mocking up a packaging test that would at least do a smoke test.

(Also, at the moment, the c++ interface relies on a f2018 support (for CFI_establish) and this needs to be tested for.)

Do we need specific flag to enable it?

MehdiChinoune · 2024-06-10T10:30:33Z

CMakeLists.txt

+	set(CMAKE_INSTALL_MODULEDIR ${_DEFAULT_CMAKE_INSTALL_MODULEDIR}
+			CACHE STRING
+			"Fortran module installation path (Not a cmake native variable)"
+	)
+	cmake_path(IS_ABSOLUTE CMAKE_INSTALL_MODULEDIR _is_absolute)
+	if (_is_absolute)
+		set(CMAKE_INSTALL_FULL_MODULEDIR ${CMAKE_INSTALL_MODULEDIR})
+	else ()


It's not a good idea to use CMAKE_ prefixed variables (To avoid a possible future conflict).
I think WANNIER90_INSTALL_MODULEDIR is better.

Fully aware of that. The issue is that CMake install support for Fortran has not been implemented and these are a bit tricky because these are ABI dependent and there is no discussion on how it should be designed. This path is also one that should be common among all dependencies, thus the preference of CMAKE_ variable.

Currently this is made compatible with fortran-stdlib in hopes that there can be convergence towards that.

MehdiChinoune · 2024-06-10T10:34:06Z

cmake/wannier90.pc.in

+prefix: @CMAKE_INSTALL_PREFIX@
+
+Name: wannier
+Version: @PROJECT_VERSION@
+Description: @PROJECT_DESCRIPTION@
+URL: @PROJECT_HOMEPAGE_URL@
+
+Cflags: -I@CMAKE_INSTALL_FULL_MODULEDIR@ -I@CMAKE_INSTALL_FULL_INCLUDEDIR@
+Libs: -L@CMAKE_INSTALL_FULL_LIBDIR@ -lwannier90


Suggested change

prefix: @CMAKE_INSTALL_PREFIX@

Name: wannier

Version: @PROJECT_VERSION@

Description: @PROJECT_DESCRIPTION@

URL: @PROJECT_HOMEPAGE_URL@

Cflags: -I@CMAKE_INSTALL_FULL_MODULEDIR@ -I@CMAKE_INSTALL_FULL_INCLUDEDIR@

Libs: -L@CMAKE_INSTALL_FULL_LIBDIR@ -lwannier90

prefix: @CMAKE_INSTALL_PREFIX@

libdir: ${prefix}/@CMAKE_INSTALL_LIBDIR@

includedir: ${prefix}/@CMAKE_INSTALL_INCLUDEDIR@

moduledirL: ${prefix}/@WANNIER90_INSTALL_MODULEDIR@

Name: wannier

Version: @PROJECT_VERSION@

Description: @PROJECT_DESCRIPTION@

URL: @PROJECT_HOMEPAGE_URL@

Cflags: -I${moduledir} -I${includedir}

Libs: -L${libdir} -lwannier90

Using full paths make it unrelocatable.

Indeed, there was some context for this, but I completely lost it. Maybe it was related to testing the pkg-config file in the integration tests since the file in the build directory is not usable. Really don't remember why I made it like that.

There are more issues there though because PUBLIC linked libraries are not included. In principle this should be written as:

file(GENERATE OUTPUT wannier.pc.in INPUT wannier.pc.in.in) configure_file(${CMAKE_CURRENT_BINARY_DIR}/wannier.pc.in wannier.pc)

And use a gen-ex depending on the target property to get the -l/-L/-I flags needed there. Didn't get the time to investigate what are appropriate flags for it though, would appreciate some help/discussion on this.

This is necessary because of the use mpi_f08 that is conditionally used in the Fortran module api.

You could add a new string variable WANNIER_LINK_LIBRARIES and add any library wannier is linked to. then add Requires: @WANNIER_LINK_LIBRARIES@ to wannier.pc.in file.
but WANNIER_LINK_LIBRARIES should contain the names of their .pc files.

The question is where do we get the values to construct it? If we figure out that, I can write a prototype to be more templatable.

These values are taken from targets like MPI::MPI_Fortran. We can investigate that is populate in there with things like: https://stackoverflow.com/a/34292622. It should be in an *_INTERFACE_* like property. But maybe target Wannier_lib should be used to check that instead

CMakeLists.txt

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

LecrisUT mentioned this pull request Feb 21, 2024

Cmake #454

Closed

LecrisUT force-pushed the cmake branch 2 times, most recently from c1f6922 to 002bdab Compare February 21, 2024 13:46

LecrisUT commented Feb 21, 2024

View reviewed changes

LecrisUT force-pushed the cmake branch from 3015cd2 to d481948 Compare February 21, 2024 16:51

LecrisUT force-pushed the cmake branch from d481948 to 62bcd83 Compare February 22, 2024 09:06

LecrisUT commented Feb 22, 2024

View reviewed changes

LecrisUT force-pushed the cmake branch 2 times, most recently from 388e5e4 to 7540c6f Compare February 22, 2024 10:43

LecrisUT force-pushed the cmake branch 2 times, most recently from d3eaa3d to bf954ab Compare February 22, 2024 13:10

LecrisUT mentioned this pull request Feb 22, 2024

Fedora packaging #444

Open

LecrisUT force-pushed the cmake branch from bf954ab to 0307c25 Compare February 23, 2024 10:38

MehdiChinoune reviewed Jun 10, 2024

View reviewed changes

MehdiChinoune reviewed Jun 11, 2024

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

CMake implementation

36d0ec4

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

LecrisUT force-pushed the cmake branch from 0307c25 to 76dd461 Compare June 11, 2024 09:11

LecrisUT added 4 commits June 11, 2024 11:25

Reconfigure GH Actions

46d2fd5

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

[WIP] Fix demo.F90

63a7eba

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

Fedora packaging convention

2371623

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

Fix python shebang

07fd984

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

LecrisUT force-pushed the cmake branch from 60cde63 to 7289169 Compare June 11, 2024 09:30

Fix executable suffix

15188ba

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

LecrisUT force-pushed the cmake branch from 7289169 to 2393d90 Compare June 11, 2024 09:39

LecrisUT added 2 commits June 11, 2024 11:52

Fix MPI support

9e992d2

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

Make wannier90.pc more relocatable

ef03aef

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

LecrisUT force-pushed the cmake branch from 2393d90 to ef03aef Compare June 11, 2024 09:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cmake #487

Cmake #487

LecrisUT commented Feb 21, 2024

LecrisUT left a comment

LecrisUT Feb 21, 2024

LecrisUT Feb 21, 2024

qiaojunfeng Feb 21, 2024

LecrisUT Feb 21, 2024

qiaojunfeng Feb 21, 2024

LecrisUT Feb 21, 2024

LecrisUT Feb 21, 2024

LecrisUT Feb 21, 2024

LecrisUT commented Feb 21, 2024

LecrisUT Feb 22, 2024

LecrisUT commented Feb 22, 2024

JeromeCCP9 commented Feb 22, 2024

JeromeCCP9 commented Feb 23, 2024

LecrisUT commented Feb 23, 2024

MehdiChinoune Jun 10, 2024 •

edited

Loading

LecrisUT Jun 10, 2024

MehdiChinoune Jun 10, 2024

LecrisUT Jun 10, 2024

MehdiChinoune Jun 10, 2024 •

edited

Loading

LecrisUT Jun 10, 2024 •

edited

Loading

		@@ -0,0 +1,179 @@
		cmake_minimum_required(VERSION 3.25...3.29)

		@@ -0,0 +1,7 @@
		cmake_minimum_required(VERSION 3.25...3.29)
		project(demo_Wannier90 LANGUAGES Fortran)

Cmake #487

Are you sure you want to change the base?

Cmake #487

Conversation

LecrisUT commented Feb 21, 2024

LecrisUT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LecrisUT commented Feb 21, 2024

Choose a reason for hiding this comment

LecrisUT commented Feb 22, 2024

JeromeCCP9 commented Feb 22, 2024

JeromeCCP9 commented Feb 23, 2024

LecrisUT commented Feb 23, 2024

MehdiChinoune Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MehdiChinoune Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

LecrisUT Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

MehdiChinoune Jun 10, 2024 •

edited

Loading

MehdiChinoune Jun 10, 2024 •

edited

Loading

LecrisUT Jun 10, 2024 •

edited

Loading