Each component can now be built and installed separately

What is the new feature

The implementation allowed for separate building and installation of all the components, enhancing efficiency and modularity

How is it implemented

Created separate target files for individual components; also added new CMake options to enable the building of individual components.

Fixed an issue with the 'libiconmath' package not being found in another program when it is installed in a custom prefix directory.

Additionally, two more major changes were made:

• In this commit, I used different directories to install the mod files of different components of iconmath. The motivation behind the change was that I found a code that uses a module from iconmath::horizontal compiles fine even though that component is not linked using target_link_libraries.
• In this commit, I changed the way target files of individual components are included when a new project tries to find iconmath. Before, the inclusions of components interpolation and horizontal were done depending on their presence in the proper place. But, because of this, it was possible to use target_link_libraries(new-project PRIVATE iconmath::horizontal) successfully even when iconmath was found using find_package(iconmath CONFIG REQUIRED COMPONENTS interpolation). This should always give an error, which was not the case before these changes were made.

Mandatory steps before review

• Gitlab CI passes (Hint: use make format for linting)
• New feature is covered by additional unit tests
• Mark the merge request as ready by removing Draft:

Mandatory steps before merge

• Reviewed by a maintainer
• Incorporate review suggestions
• Prior to merging, please remove any boilerplate from the MR description, retaining only the Please describe your feature in a couple of words and describe important implementation details of the feature section to maintain

You are not supposed to merge this request by yourself, the maintainers of libiconmath take care of this action!

requested review from @b382190

assigned to @k202170

marked the checklist item Gitlab CI passes (Hint: use make format for linting) as completed

marked the checklist item Mark the merge request as ready by removing Draft: as completed

I used the following CMakeLists.txt file to verify if 'iconmath' could be located by an external project and to ensure that all components could be linked properly.

cmake_minimum_required(VERSION 3.18)
project(TestIconMathProject Fortran)

# Find the iconmath package, requiring the interpolation component
find_package(iconmath CONFIG REQUIRED COMPONENTS interpolation)

# Create an executable from your test source file
add_executable(test_interpolation src/main.f90)

# Link the iconmath::interpolation target to your executable
target_link_libraries(test_interpolation PRIVATE iconmath::support)
target_link_libraries(test_interpolation PRIVATE iconmath::interpolation)
target_link_libraries(test_interpolation PRIVATE iconmath::horizontal)

The program main.f90 utilizes modules from the library.

I think you don't need this?

changed this line in version 2 of the diff

Maybe add checks for library dependencies.
interpolation requires support, horizontal requires inerpolation and support.

This won't be necessary because of this

Maybe use 1.0.0 already so that we don't have to open another MR to change this.

changed this line in version 3 of the diff

added 4 commits

• 8c176361 - build: removed redundant dependency to a cmake package in config.cmake.in
• fb883f78 - build: installed mod files from different components into different directories inside include
• e4dfd148 - build: loading of target files of individual components is made dependent on FIND_COMPONENTS
• 285a4d21 - chore: fixed cmake linting issues

Compare with previous version

added 1 commit

• f09d64f4 - chore: changed the version of the library

Compare with previous version

There are two major changes done since the last review:

• In this commit, I used different directories to install the mod files of different components of iconmath. The motivation behind the change was that I found a code that uses a module from iconmath::horizontal compiles fine even though that component is not linked using target_link_libraries.
• In this commit, I changed the way target files of individual components are included when a new project tries to find iconmath. Before, the inclusions of components interpolation and horizontal were done depending on their presence in the proper place. But, because of this, it was possible to use target_link_libraries(new-project PRIVATE iconmath::horizontal) successfully even when iconmath was found using find_package(iconmath CONFIG REQUIRED COMPONENTS interpolation). This should always give an error, which was not the case before these changes were made.

Can you add these in the description?

I now think maybe it is better to link the required components automatically rather than showing an error that something is missing. Maybe we can print a message, for example, that math-support is required for math-interpolation and link it automatically.

I do not think that is the right practice. Any developer should know which libraries should be linked, and if done in the wrong way, our code should throw an error instead of correcting their mistakes.

Actually, I implemented what you mentioned here first, but changed it to how it is now because of this argument.

Well, you also have a point. My argument is that linking automatically should not break the code.
It might be a bit hard to ask everyone to know beforehand that you have to build everything together if you want to use a module in math-horizontal.
We can bring it up on Thursday if you are not in a hurry.

I think the best person to ask would be Sergey. I am not in a hurry, though.

@m300488 Do you think the math libraries should link all required components automatically or should we show an error and ask people to build the missing components?

We are talking about this change: !5 (diffs)

added 1 commit

• 69c93bc1 - style: formatted the cmake codes as suggested my make-format

Compare with previous version

mentioned in merge request !6 (merged)

changed the description

At the moment, several things here look like overengineering to me. I will try to come up with a proper review soon.

Why do we need separate targets files for the components?

I thought that each component must have its own target file. That's why I started to make these changes. It makes sense to me, as it helps all the components be managed independently. Although it is a small project now, it has the possibility to grow, and then having smaller target files for each component will also scale well. I am not sure what the benefits of having just one target file for all components are other than that it will be simpler.

Yes, it will be simpler. What are the benefits of having multiple target files?

Each component has its own directory, 'cmake' configuration, and binary file. I do not see how having its target file breaks the deal. For me, it is still part of the modular structure upon which the library is built, and enough reason to have it this way.

But still, I would like to discuss a problem which could be solved by having three targets. I already mentioned a CMakeLists.txt file in one of my earlier comments in this MR. It is:

cmake_minimum_required(VERSION 3.18)
project(TestIconMathProject Fortran)

# Find the iconmath package, requiring the interpolation component
find_package(iconmath CONFIG REQUIRED COMPONENTS interpolation)

# Create an executable from your test source file
add_executable(test_interpolation src/main.f90)

# Link the iconmath::interpolation target to your executable
target_link_libraries(test_interpolation PRIVATE iconmath::support)
target_link_libraries(test_interpolation PRIVATE iconmath::interpolation)
target_link_libraries(test_interpolation PRIVATE iconmath::horizontal)

I think you will agree that this should produce an error during the configuration, mainly because it only asks for the component interpolation and its dependencies at the find_package step. Still, later, it is trying to link the horizontal component (one should not be just lucky about it). But, with one target file and assuming iconmath with all of its components being already installed, this does configure without any error. Actually, it was still possible to configure it without failing, even with three target files, till I corrected it in (this)[!5 (e4dfd148)] commit. Do you see that as a benefit? I can not think of an alternative but elegant solution for this, which could be my lack of understanding of CMake.

For me, it is still part of the modular structure upon which the library is built, and enough reason to have it this way.

The "modular structure" is not an absolute good but a tradeoff with the usual benefit of improved maintainability. Changes in this particular file do not look like they make the code more maintainable. Simply because a single target file does not need any extra code to maintain.

I think you will agree that this should produce an error during the configuration, mainly because it only asks for the component interpolation and its dependencies at the find_package step. Still, later, it is trying to link the horizontal component (one should not be just lucky about it).

I am sorry but you are starting with a wrong assumption. The users are free to do whatever they want in their scripts (believe it or not, sometimes the reason for that is that they know better). We are only supposed to deliver the interface: the user requests a component - we define the respective target (or fail if the target is not available but REQUIRED). We are free to define more targets, functions and variables but those are implementation details. If the users rely on horizontal being available when only interpolation is requested, it's their problem. This might break at some point and will need a fix, which will be to explicitly request horizontal as well.

Installing modules to a subdirectory does not look like a good idea. I'd say, it brings more problems than it solves. Let's re-iterate this later if you insist.

Without this, we will be exposing modules from other components even when they are not needed. However, the initial motivation behind the change was that I found it possible for a code using a module from iconmath::horizontal to compile without any error, even though that component is not linked using target_link_libraries.

Why is that bad? Even if it is, why is that one of many things that can go wrong in the user's build environment that concerns you so badly that you decide to go for this complication? Note that this decision might solve problems for some users but introduces counter-intuitive requirements for all of them (i.e. go figure one needs -I/prefix/include/horizontal instead of -I/prefix/include).

It is not 'things that can go wrong in user's build environment' that concerns me. In my humble opinion, putting all the module files coming from different Fortran-based libraries in $prefix/include/ is not the correct design. Again, I will go back to TestIconMathProject. Let's say that it could be configured successfully in its current form (which, again, should not happen). Furthermore, one really wants to use modules from horizontal and has these lines in main.f90

    use mo_lib_intp_rbf  
    use mo_lib_divrot

where mo_lib_divrot is from horizontal. Again, I am assuming you would agree with me that this should not be compiled successfully. But it gets compiled when all the modules are placed in $prefix/include because the compile command has the -I$prefix/include flag in it. That is the design flaw I was talking about, and that's why I decided to have different directories for each component.

About the difficulty that the user would face in correctly adding the path to the modules, it is taken care of by INSTALL_INTERFACE in the parent project, and the path is automatically added when the new package is configured using CMake. Isn't it? I think the problem you mentioned would arise from other configuration options. Let's say we are promoting configuration via CMake, then.

BTW, the path is -I/prefix/include/iconmath/horizontal. Don't get mad at me! Again, it could be my lack of understanding of CMake, but I wanted to solve the issue, and I could not find an elegant solution.

putting all the module files coming from different Fortran-based libraries in $prefix/include/ is not the correct design

Yes, because, according to almost any piece of documentation out there, $prefix/include is for C headers — nothing about Fortran module files (or even C++ headers). For that reason, some packages install the modules to $prefix/lib. That's a detour though. The question is, of course, whether the module files should be installed in one directory or to several subdirectories.

Again, I am assuming you would agree with me that this should not be compiled successfully.

I don't understand why you are intentionally trying to break something that works. What bigger problem are you trying to protect the users and/or yourself from? Also, if your application uses a Fortran library (e.g. horizontal), which depends on another Fortran library (e.g. support), you are supposed to provide -I flags for both of them anyway. So, if an application explicitly depends on horizontal only, some of its parts can still use support modules and compile successfully. So, the issue is still there but with the extra overhead for everyone.

In my opinion, what would really help is the proper naming of the modules.

What is the scenario when this code is of any use? We just add_subdirectory in the right order in the parent CMakeLists.txt, don't we?

In the case that someone mistakenly set -DBUILD_ICONMATH_SUPPORT as OFF. But I agree with your next point that -DBUILD_ICONMATH_SUPPORT is completely redundant. So, this is also redundant and should be changed back. May be, for iconmath::horizontal, checking whether iconmath::interpolation target exists still makes sense!? Or, may be, as you said, I am overengineering things here.

If some of the option combinations are invalid (for whatever reason), we should directly (i.e. not by checking whether a TARGET is available) check that as early as possible and fail.

What is the use case for -DBUILD_ICONMATH_SUPPORT=OFF?

None, should not be there, I agree.