Thanks for taking this on. I'll take a look at the precommit failure.

Could you please remove the intermediate src folder, e.g. move src/grpp/src/angular_integrals.c to src/grpp/angular_integrals.c.

A few more comments, which could also be addressed in follow up PRs:

1. I'm fine with keeping the MIT license, but I'd add our usual header for the CP2K developers group to every file.
2. The build flags __LIBGRPP and CP2K_USE_GRPP should be remove because in the future libgrpp will always be present.
3. We should check our coverage report to see which parts of the library we actually need and remove any unused code.
4. We should also integrate the tests from libgrpp either as unittests or libtests.
5. I'm a bit worried about name collisions during linking. Is there a way to ensure that the grpp directory gets linked late?

Tagging @aoleynichenko just in case.

Thanks for taking this on. I'll take a look at the precommit failure.

Could you please remove the intermediate src folder, e.g. move src/grpp/src/angular_integrals.c to src/grpp/angular_integrals.c.

A few more comments, which could also be addressed in follow up PRs:

1. I'm fine with keeping the MIT license, but I'd add our usual header for the _CP2K developers group_ to every file.

We have no choice as we are not the author of this library.

2. The build flags `__LIBGRPP`  and `CP2K_USE_GRPP` should be remove because in the future libgrpp will always be present.

I will wait before suppressing this as well. I think we should treat it as a dependency for now.

3. We should check our [coverage](https://www.cp2k.org/static/coverage/) report to see which parts of the library we actually need and remove any unused code.

4. We should also integrate the tests from [libgrpp](https://github.com/aoleynichenko/libgrpp) either as [unittests](https://github.com/cp2k/cp2k/blob/master/tests/UNIT_TESTS) or [libtests](https://github.com/cp2k/cp2k/tree/master/tests/LIBTEST).

I need to understand what these tests do and what they are reporting before doing this. That's why I also consider this as experimental feature and that the option CP2K_USE_GRPP should stay until we sort these things out. For now, we need to be sure it builds with cmake and spack. The toolchain won't be an issue.

5. I'm a bit worried about name collisions during linking. Is there a way to ensure that the grpp directory gets linked late?

Why ? I can compile the entire source without any conflict at all otherwise I would have fixed it. It also does not matter if the code is compiled first or second or third. I do not create a separate archive for this as we do not indent to distribute it separately. It is directly included libcp2k.(a/so)

Tagging @aoleynichenko just in case.

We have no choice as we are not the author of this library.

Yes, we should certainly honor the original license. The way you did it now is exactly what I had in mind.

I will wait before suppressing this as well. I think we should treat it as a dependency for now.

Sure, let's take it one step at a time. It's not every day that we scoop up 60 kLoC.

It also does not matter if the code is compiled first or second or third.

Well, but it does matter in which order the objects get linked together, doesn't it? We usually protect against name collision by using modules in Fortran and prefixes like dbm_ in C. However, the libgrpp codes does not use prefixes. It therefore adds a ton of symbols to the flat (!) linker namespace. And many of these have rather generic names like binomial. So, I think it would be good if we instructed CMake to link the objects from the grpp directory only at the every end.

We have no choice as we are not the author of this library.

Yes, we should certainly honor the original license. The way you did it now is exactly what I had in mind.

I will wait before suppressing this as well. I think we should treat it as a dependency for now.

Sure, let's take it one step at a time. It's not every day that we scoop up 60 kLoC.

It also does not matter if the code is compiled first or second or third.

Well, but it does matter in which order the objects get linked together, doesn't it? We usually protect against name collision by using modules in Fortran and prefixes like dbm_ in C. However, the libgrpp codes does not use prefixes. It therefore adds a ton of symbols to the flat (!) linker namespace. And many of these have rather generic names like binomial. So, I think it would be good if we instructed CMake to link the objects from grpp directory only at the every end.

that's certainly true for shared libraries. The best is to do a proper namespacing and hide internal functions. The outside world interface is already namespace.

Dear Alexander, thanks a lot for chiming in! It's very comforting to have your explicit approval. I hope you also agree with our conclusion that absorbing libgrpp into CP2K is the best way to keep your great work alive?

• all external functions are namespaced.
• [partial] internal functions are declared static (still need some work)
• remove warnings
• cp2k license header (on top of the original header)
• still need to include the tests in the unit tests

Looking at the coverage report, it seems we just need a few files from libgrpp. Should we already drop the unused files?

OK, let me check that everything works fine without the unnecessary code. The only issue I could see is that some path might not be taken when we run the code but still needed at compilation time.

I am inclined of merging this PR and then open another PR for removing the unnecessary code. I did some simple tests removing the unneeded files from the compilation but got a long list of errors during linking time which means that I have to go through the entire code manually to remove all unnecessary functions. If we choose this path I wonder if it is not better to just rewrite the functionalities we need while keeping the author copyright.

this test is failing but I do not understand why. It does not depend on GRPP a priori

/opt/cp2k/build/bin/TEST-2025-02-24_12-07-56/QS/regtest-gpw-8/si8_broy_wc.inp.out
Difference too large: 7.16e-04 > 2e-06, ref_value: 1902.1967139926, value: 1900.8347065463.

I don't think it makes sense to identify and delete unused code in the library. 1) Maybe somebody wants to use the deleted functionality in the future 2) We may replace the currently used functionalty by our own function In any case it is a lot of work with only minor gains.

that is also my conclusion. Both arguments make sense but it is a lot of work to do.

@oschuett or anyone with merge access has the final call for this PR. I am currently checking why this test fails.

Alright, then I'll merge this as is. The si8_broy_wc.inp test is a notoriously flaky.

Thanks @oschuett .

Great work! I've created #4036 so we don't forget about it.