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.

CMakeLists.txt

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.

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

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

LecrisUT · 2025-02-09T20:42:05Z

@JeromeCCP9 Can you check this PR again? I have disabled tests that fail, but you can work on it gradually in subsequent PRs. It would be great to get this in, because I plan to package this for Fedora soon because of quantum-espresso dependency

JeromeCCP9 · 2025-02-10T12:02:10Z

Hi Christian! Yes, surely we'll work on this and include it in 4.0. For me the main concern was that the configuration files were a bit too complex and complicated, but let me go through it and we'll discuss later.

Thank you for revisiting this! It is good timing. Jerome

JeromeCCP9

Provides rich cmake support for Wannier90!

Signed-off-by: Cristian Le <[email protected]>

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

test-suite/libv2-demo/lib-gas-opt-pllel/CMakeLists.txt Outdated Show resolved Hide resolved

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 Show resolved Hide resolved

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

LecrisUT force-pushed the cmake branch 5 times, most recently from 13df820 to e26ac57 Compare February 9, 2025 19:51

LecrisUT force-pushed the cmake branch 3 times, most recently from 92633c8 to 74827e6 Compare February 9, 2025 20:32

JeromeCCP9 self-requested a review March 10, 2025 12:26

JeromeCCP9 approved these changes Mar 10, 2025

View reviewed changes

JeromeCCP9 force-pushed the cmake branch 2 times, most recently from ecf0969 to f0f72e7 Compare March 10, 2025 17:01

LecrisUT and others added 8 commits March 10, 2025 17:22

CMake implementation

bdcfdb1

Signed-off-by: Cristian Le <[email protected]>

Reconfigure GH Actions

262f03f

Signed-off-by: Cristian Le <[email protected]>

Fix python shebangs

bca34d6

Add functional tests

0ee094a

Comments on C library test

fbe9d8a

Switch to using MPI90 until tests are adapted

7bc0fb0

Disable library-mode tests also

4d41315

Disable flaky test

0c07e1f

JeromeCCP9 force-pushed the cmake branch from f0f72e7 to 0c07e1f Compare March 10, 2025 17:22

jryates merged commit 3bfc300 into wannier-developers:develop Mar 10, 2025
1 of 2 checks passed

LecrisUT deleted the cmake branch July 8, 2025 19:43

Cmake #487

Cmake #487

Uh oh!

Conversation

LecrisUT commented Feb 21, 2024

Uh oh!

LecrisUT 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!

Uh oh!

Uh oh!

Uh oh!

LecrisUT commented Feb 21, 2024

Uh oh!

Uh oh!

LecrisUT commented Feb 22, 2024

Uh oh!

JeromeCCP9 commented Feb 22, 2024

Uh oh!

JeromeCCP9 commented Feb 23, 2024

Uh oh!

LecrisUT commented Feb 23, 2024

Uh oh!

MehdiChinoune Jun 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

MehdiChinoune Jun 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LecrisUT Jun 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LecrisUT commented Feb 9, 2025

Uh oh!

JeromeCCP9 commented Feb 10, 2025

Uh oh!

JeromeCCP9 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

MehdiChinoune Jun 10, 2024 •

edited

Loading

MehdiChinoune Jun 10, 2024 •

edited

Loading

LecrisUT Jun 10, 2024 •

edited

Loading