Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[new package] cminpack #21025

Closed
wants to merge 4 commits into from
Closed

Conversation

luau-project
Copy link
Contributor

Description

According to the description given by the author Frédéric Devernay on https://github.com/devernay/cminpack, cminpack is a C/C++ rewrite of the MINPACK software (originally in FORTRAN) for solving nonlinear equations and nonlinear least squares problems.

Positive points

Website

http://devernay.free.fr/hacks/cminpack/

Comment on lines 20 to 23
# on Windows, EOL must be CRLF (\r\n)
# for reference test files,
# but the tarball is packed with LF (\n).
# So, we replace LF with CRLF.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really needed? Most things can deal with LF nicely nowadays.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, in the current CMake build present on the released tarball, it is.

However, due your valuable input, I already solved this puzzle on a fork and I am submitting a PR on cminpack's repository.

-G Ninja \
-DCMAKE_INSTALL_PREFIX="${MINGW_PREFIX}" \
"${extra_config[@]}" \
-DUSE_BLAS=OFF \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason BLAS isn't used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you look at Ubuntu's page for cminpack https://packages.ubuntu.com/source/noble/cminpack, there is no dependency on BLAS. So, I choose to disable it as well.

@lazka
Copy link
Member

lazka commented May 30, 2024

The static libs have "_s" suffix, which likely means they won't work when used with pkg-config at least. Not sure what's this about and how much of an issue that is.

@luau-project
Copy link
Contributor Author

@lazka

The static libs have "_s" suffix, which likely means they won't work when used with pkg-config at least. Not sure what's this about and how much of an issue that is.

You have given really helpful tips. I confirmed that the current files generated for pkg-config on CMake build are not as they would be. So, looks like you've found an issue on cminpack build with CMake. Locally, I addressed this issue as well and I am submitting another PR on cminpack's repository.

Once I get a response about all these PRs, I'll get back and drop updates here. Thank you.

@lazka
Copy link
Member

lazka commented Jun 3, 2024

You can also "backport" patches, as long as you link to the upstream PR. But waiting is also an option of course.

@luau-project
Copy link
Contributor Author

Updates

Hello, @lazka

I'll comment the changes I have made to the cminpack insertion:

  1. Added a patch file (001-fix-cmake-eol-for-tests.patch) to avoid EOL problems, which was submitted and approved (but not yet merged) as upstream PR fix: EOL problems running CMake tests on Windows devernay/cminpack#63;
  2. Added another patch file (002-fix-pkg-config.patch) to fix some pkg-config related issues that you noted on your previous post. This patch was also submitted as PR in the upstream repo, but it was not analyzed yet fix: generated pkg-config (.pc) files on CMake build devernay/cminpack#64;
  3. Removed the static library version. Reason: At the current state with these 2 patches, pkg-config files are fixed, but the resulting CMake files for both shared and static libraries would not be ok (static would be broken). In order to fix CMake files for the static library version, it is required a major rework of their CMake build. In the future, I plan to help them in this area, but it is a long journey. In short, for now, static library is disabled;
  4. Followed the same procedure on Debian distros, which builds and installs only the double-precision version of the library (-DCMINPACK_PRECISION=d on CMake configuring step).

@lazka
Copy link
Member

lazka commented Jun 15, 2024

Followed the same procedure on Debian distros, which builds and installs only the double-precision version of the library (-DCMINPACK_PRECISION=d on CMake configuring step).

I don't see anything like that here: https://salsa.debian.org/science-team/cminpack/-/blob/master/debian/rules

@luau-project
Copy link
Contributor Author

Reply

Followed the same procedure on Debian distros, which builds and installs only the double-precision version of the library (-DCMINPACK_PRECISION=d on CMake configuring step).

I don't see anything like that here: https://salsa.debian.org/science-team/cminpack/-/blob/master/debian/rules

This CMINPACK_PRECISION argument was introduced on version 1.3.8, which you can compare analyzing the commits of the released tarballs 1.3.7 and 1.3.8. On the other hand, Debian repositories uses 1.3.6, which back in the time, it was building only the double precision version (can be confirmed by the installed files on Debian judging by the absence of suffix in the library name, discussed below).

A bit of context

In the current version 1.3.9, the argument CMINPACK_PRECISION has 4 possible values (s, d, ld, all)

  • s: builds the single precision version (adds s suffix to the library name, e.g.: libcminpacks.dll);
  • d: builds the double precision version (CMake tests were implemented targeting this precision. This option doesn't add suffixes to the library name, e.g.: libcminpack.dll);
  • ld: builds the long-double precision version (adds ld suffix to the library name, e.g.: libcminpackld.dll);
  • all: builds all the previous precisions (default nowadays when CMINPACK_PRECISION is not provided).

Motivation to use only the double precision version

The main motivation to use only the double precision version of cminpack is due the fact that CMake tests were designed to run for that precision, and that precision is the version which is around for a long time. In order to avoid shipping a version "untested" by PKGBUILD check method, I sticked to that precision.

On the other hand, if you want, I can remove that CMINPACK_PRECISION argument and we take a route like Arch Linux does. Since they don't apply any CMINPACK_PRECISION argument in the configure step, they are using CMINPACK_PRECISION=all, building and delivering all the precisions.

Conclusion

Currently on version 1.3.9:

  • CMake test do test the binaries built by double-precision version;
  • CMake tests do NOT test the binaries built by single or long-double precision versions.

@luau-project luau-project marked this pull request as draft June 28, 2024 19:16
@luau-project luau-project deleted the cminpack branch November 16, 2024 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants