search

devernay / cminpack

A C/C++ rewrite of the MINPACK software (originally in FORTRAN) for solving nonlinear equations and nonlinear least squares problems
http://devernay.free.fr/hacks/cminpack/
145 stars 63 forks source link

CMake: Fix pkgconfig files #71

Closed jschueller closed 2 months ago

jschueller commented 2 months ago

I propose this simpler fixes instead of #64:

/cc @luau-project

luau-project commented 2 months ago

Hello.

I liked the idea that you used to solve the library suffix. However, there is something not working correctly.

For instance, run this shell script on your branch on a Debian-derived distro (I used Ubuntu 24.04):

cd /tmp
rm -rf cminpack
git clone --branch=pkgconfig https://github.com/jschueller/cminpack
cmake -DUSE_BLAS=OFF -DBUILD_SHARED_LIBS=ON --install-prefix $PWD/install -S cminpack/ -B _build
cmake --build _build/
ctest --test-dir _build/
cmake --install _build/
cat /tmp/install/lib/pkgconfig/cminpack.pc

You get the following screen:

Screenshot from 2024-06-28 11-50-40

In the end of the image, you can see the -D__cminpack_float__ for the double-precision version cminpack.pc.

jschueller commented 2 months ago

indeed, PC_CMINPACK_CFLAGS must be reset, fixed now

luau-project commented 2 months ago

Comments

On the tests I ran after your fixed PC_CMINPACK_CFLAGS, I didn't find any issues related to the generated pkgconfig files, nor the CMinpackConfig scripts.

I think your PR has a cleaner solution to fix pkgconfig files while keeping compatibility to previous versions. If the main intent is to fix pkgconfig related things, I think your PR is ready to be merged.

Extra (could be addressed on subsequent PRs or newer commits)

In my PR, I also added a guard to only test the library if the double-precision version was built. Because you can see here https://github.com/jschueller/cminpack/blob/pkgconfig/examples/CMakeLists.txt#L33 and here https://github.com/jschueller/cminpack/blob/pkgconfig/examples/CMakeLists.txt#L65, it is testing only for the double precision version.

jschueller commented 2 months ago

done too

luau-project commented 2 months ago

I didn't test, but I guess the same target guard is needed here https://github.com/jschueller/cminpack/blob/pkgconfig/examples/CMakeLists.txt#L47

jschueller commented 2 months ago

indeed

luau-project commented 2 months ago

In my view, I think your PR is now ready to be merged.

devernay commented 2 months ago

Why don't we get full CI build on this PR? The whole ubuntu matrix is missing

jschueller commented 2 months ago

github is having troubles: https://www.githubstatus.com/