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

[lapack-reference] CMake-Config script references wrong import library on Windows #38009

Closed
ulfllorenz opened this issue Apr 6, 2024 · 8 comments · Fixed by #38035
Closed
Assignees
Labels
category:port-bug The issue is with a library, which is something the port should already support

Comments

@ulfllorenz
Copy link
Contributor

Environment

  • x64-windows, standard MSVC toolchain

Describe the bug

I am currently not able to use the built lapack-reference through CMake, because the config script references a non-existing file.
In particular, the file "installed\x64-windows\share\lapack-3.11.0\lapack-targets-debug.cmake" references an import library through

set_target_properties(lapack PROPERTIES
IMPORTED_IMPLIB_DEBUG "${_IMPORT_PREFIX}/debug/lib/liblapack${CMAKE_IMPORT_LIBRARY_SUFFIX}"
IMPORTED_LOCATION_DEBUG "${_IMPORT_PREFIX}/debug/bin/liblapack.dll"
)

However, the import library under installed/x64-windows/debug/lib is called "lapack.lib" without the "lib"-prefix. The same goes for the release target. Trying to include the lapack library through CMake's config mode fails then, because lapack-targets.cmake executes a file existance check around line 78 that does not pass. Curiously, the dll is called "liblapack.dll" in sync with the config script.

Suggested solution: Either rename the import libraries or change the config mode scripts to use the correct name.

@dg0yt
Copy link
Contributor

dg0yt commented Apr 6, 2024

Just a quick check please:

  • All installed package updated.
  • No CMake 3.29.1.

@jimwang118 jimwang118 added category:question This issue is a question category:port-bug The issue is with a library, which is something the port should already support and removed category:question This issue is a question labels Apr 7, 2024
@jimwang118
Copy link
Contributor

I've reproduced the issue locally and will fix it.

@Neumann-A
Copy link
Contributor

If you want to use lapack use module mode instead of config mode.

@ulfllorenz
Copy link
Contributor Author

ulfllorenz commented Apr 8, 2024

I would disagree with that suggestion

  • Config mode works well, except for this easy to spot problem. If I manually patch the target*.cmake files, everything just works fine.
  • CMake comes with a module for searching LAPACK that is almost guaranteed to not find the vcpkg installation. You can override this (I think lapack-reference comes with an own module), but then you come to issue number 2: The LAPACK module is very pecular and my experience with it is generally poor; at the very least this solution is terribly brittle.

@Neumann-A
Copy link
Contributor

Config mode works well

unless you want to change the lapack implementation, e.g. to mkl or other providers.

CMake comes with a module for searching LAPACK that is almost guaranteed to not find the vcpkg installation.

Why do you think that? Any prove or are you just saying that to make up an argument?

The LAPACK module is very pecular and my experience with it is generally poor; at the very least this solution is terribly brittle.

In the vcpkg world it shouldn't be brittle and just work as expected.

@ulfllorenz
Copy link
Contributor Author

Two concessions: I am only consumer of a library that uses LAPACK, so I can see the madness while debugging this third-party source code, but have not had first-hand experience. And I freely confess that the issue is difficult in principle, and seems worse in practice.

If you want to use MKL, it provides an own MKLConfig.cmake script that sets up a target MKL::MKL. That makes the client code more complex (ifs instead of find_package(LAPACK)), but you have to cater for all sorts of implementation-specific issues anyway (installation rules etc.), so in practice you would not support arbitrary LAPACK but (lapack-reference | MKL | ...).

I have tried out and looked into the FindLAPACK module over the years and was generally not impressed; I understand why it works the way it works, but found it, let's say, difficult to use to the point where switching LAPACK libraries only works if you know the ins and outs of these LAPACK libraries in the first place. But that is already some time ago and the only thing left is a general feeling of things not working well.

In practice, to use FindLAPACK.cmake, I need to either override the module search path, so that a vcpkg-patched FindLAPACK.cmake is found instead of the CMake-provided one, or I need to spoon-feed FindLAPACK with the compiler settings to link against my LAPACK library. Both approaches are what I call brittle.

@dg0yt
Copy link
Contributor

dg0yt commented Apr 9, 2024

I have tried out and looked into the FindLAPACK module over the years and was generally not impressed;

Years: yes, not impressed: wait: #24327 was just merged.

@Neumann-A
Copy link
Contributor

I need to either override the module search path, so that a vcpkg-patched FindLAPACK.cmake is found instead of the CMake-provided one, or I need to spoon-feed FindLAPACK with the compiler settings to link against my LAPACK library.

That is not of vcpkg works. As I said: It should just work if you use the vcpkg toolchain.

Otherwise I would advise you to use something like FindWrapLAPACK.cmake which delegates to an implementation via an INTERFACE target like Wrap::LAPACK. Do not use the installed config targets directly. This also keeps complexity within FindWrapLAPACK instead of all over the codebase.

JavierMatosD pushed a commit that referenced this issue Apr 24, 2024
…g cmake under Windows (#38035)

Fixes #38009
Remove the statement renaming liblapack.lib.
- [X] Changes comply with the [maintainer
guide](https://github.com/microsoft/vcpkg-docs/blob/main/vcpkg/contributing/maintainer-guide.md).
- [ ] ~~SHA512s are updated for each updated download.~~
- [ ] ~~The "supports" clause reflects platforms that may be fixed by
this new version.~~
- [ ] ~~Any fixed [CI
baseline](https://github.com/microsoft/vcpkg/blob/master/scripts/ci.baseline.txt)
entries are removed from that file.~~
- [ ] ~~Any patches that are no longer applied are deleted from the
port's directory.~~
- [X] The version database is fixed by rerunning `./vcpkg x-add-version
--all` and committing the result.
- [X] Only one version is added to each modified port's versions file.

Usage test pass with following triplets:

```
x64-windows
```

---------

Co-authored-by: Jim wang (BEYONDSOFT CONSULTING INC) <v-wangjim@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support
Projects
None yet
4 participants