cmake build: more modularity, linker fix, pkg-config fixup #556
Conversation
|
This build failure on MacOS seems to be bogus … I did not touch the Makefile build, anyway. Btw., if you are not intending to replace the Makefile build with CMake, that would be good to know. I don't insist on using CMake, but it seemed the more maintained route to getting the shared libs. |
Codecov Report
@@ Coverage Diff @@
## master #556 +/- ##
=======================================
Coverage 82.37% 82.37%
=======================================
Files 1894 1894
Lines 190679 190679
=======================================
Hits 157065 157065
Misses 33614 33614 Continue to review full report at Codecov.
|
Hi! I just re-ran the problematic MacOS test. It is all good, the problem was in Travis CI. :) About the build systems... Recently, the LAPACK's community on Github had a quite nice discussion about build systems (#488). We decided to keep both Makefile and CMake build systems active for now. Ideally, we should maintain both systems with equivalent basic compiling options. Moreover, both should be continuously tested (using Travis CI for instance). However, CMake is more versatile, and we already have some options that are only supported by the CMake build system. I think that is the case for dynamic libraries too. This being said, I don't see a problem in providing new features only for the CMake build systems. Now, if it is a typo or bug fix in the build systems, we shall also correct it on the Makefile side |
|
Oh. The -frecursive thing creeps me out. LAPACK is not thread-safe without it? Or are you just not sure? It is a significant difference in build systems if one has the flag and the other not. Why not? Of course we use OpenBLAS for serious work, but multithreaded applications are the norm and the reference should be correct … The discussion at the other issues has me confused about the current state. Is it just some tests? The tradeoff with -frecursive is just that you need big stack (ulimit -s unlimited is the way to go with any Fortran code …)? I wonder if I introduced multitreading issues in switching from Makefile to CMake; should add -frecursive to the build, then. I don't get why CMake couldn't do this, though (after testing compiler support). Pkgsrc had patches to get the shared libs out of the Makefiles before, like just about any distro, none of which ever contributed them here? And I actually detest CMake, but it seemed like the way forward here. Should we rather try to revive those patches? I guess I could add options to the Makefiles, too, and switch back. Curious. |
if(BUILD_SHARED_LIBS AND BUILD_STATIC_LIBS)There is no If you need static and shared libraries, I strongly suggest to run CMake twice because the position-independent code in a shared library differs significantly from code in static libraries and it cannot be optimized as well. For example, the compiler cannot perform inlining because it would break symbol interposition (i.e., the ability to replace library functions by using Here is a simple example demo highlighting the performance impact: Effects of Position Independent Code |
Well, I just added the option to this particular build, not CMake itself. So it is present just for the conditional you presented. This is also what your reference states. If you want to build both libs, you need to define another target. This is what the patch does. The name appears in other projects, too, it's just that it used by informal convention, not given by CMake itself.
This is of course an alternative, which needs more scripting around the build for the packager, as you cannot do a
This can be discussed. There used to be a world where you just used PIC for shared libs and tried to get away without it, even including stuff like Regarding LAPACK, I presume that if your application would benefit from inlining code from the library, your problem size is too small to actually benefit from efficient linear algebra algorithms. And then there is the point that for actual time-critical computations, you use a platform-optimized BLAS/LAPACK. Then, there is the general question whether static libraries should be built with PIC or not, see e.g.: conda-forge/boost-feedstock#11 . To combine them with code that uses PIC, you need to have them built that way or you get those friendly relocation errors. My impression was that, on 64 bit x86 at least, you tend to always use PIC, at least when packaging for general use. One might argue that one could drop the static lib at all in that case … and that it's justification is that it is bare-bones also without position independence. It's no easy topic. My addition of |
(Emphasis mine) Position-independent code inhibits inlining within the library and LAPACK might benefit from inlining certain functions a lot, e.g., xROT.
If you build a static library with position-independent code (PIC), then you have none of the benefits of shared libraries (code re-use and easy updates) and you gave up on advantages of object code (better optimization). The code optimization can be re-enabled by building the static library PIC with hidden symbols by passing the compiler option PIC in a static library is simply bad practice. Thus, the user should take care of this problem. |
|
Another reason for static libraries is to build a binary that does not depend on its build environment and can be transferred and run elsewhere, or perhaps also to have minimal overall program binary size as unused symbols are not included. Better optimization could also be a consideration. This used to be more common in the past in our use case (HPC cluster). Nowadays the number of cases where you have static versions of all needed libraries is relatively small and people like to throw around huge containers instead. I am primarily considering static libs in LAPACK out of tradition. Especially in binary distros you have the mechanism of shared BLAS/LAPACK to be able to switch implementations at runtime when you care about performance. Your point about inlining inside the library is interesting. It sounds like something stupid that is just a side-effect. I'll educate me on which functions this affects in what manner and test with differing compilers we have installed. But there is the point that a cursory check of static libs on my Ubuntu desktop system suggest that it is common practice to build also the static libs with PIC. But then … let's see what actually happens! Using this test (correct me if I'm wrong):
So … it seems to me that CMake isn't enforcing PIC here, but the toolchain of Ubuntu is? Checking what the CMake build actually does … CMake prepares different directories and builds the objects separately for static and shared builds, without PIC in the former case. So what are we arguing about? ;-) That I get PICy libraries on Ubuntu is apparently a decision they took with their toolchain to enable ASLR for all binaries. Checked the build, no PIC in compiler flags, but PICy objects. |
The compiler is applying its inlining heuristics wherever it can.
What you are seeing is code for position-independent executables (PIE for short), a feature aimed at enhancing security. PIE is enabled by default on several modern distributions including Ubuntu 20.04 but neither on CentOS 7 nor on CentOS 8 (execute
In the build output above, there is no PIC option for the static library because the static library is a separate target. If the user builds a static library with PIC code then CMake will build the exact same object code twice because of the separate targets. In this case, there is no gain from the new CMake code. It is possible to re-use the object files with an intermediate object library target but then CMake stops being able to infer whether PIC should be enabled or not if both static and shared library should be built. There is also no gain if the user wants to pass different compiler flags for the static library, for example, to enable link-time optimization or PIE code. The user has to build LAPACK twice again. I fail to see the value-add of a static library target in addition to the generic library target. In my opinion, the added code duplication does not lead to noticeable benefits for the user. |
|
I don't quite get where we differ on the diagnostics of PIE or PIC in the static libs, but let's put that aside, as it is established that the static libs resulting from my change are built correctly from your perspective, right? (Edit: I think I get it now. Thanks for pushing to me clarify my mental PIE PICture. I indeed had those terms not clearly defined. But this is irrelevant to this PR as no static lib is built with PIC here.) So all my change does is to enable a single round of to get shared and static libraries installed. It is not something unheard of to build both. I just checked a autotools/libtool-based project I maintain … does build both shared and static libs, both from separate sets of object files built with and without PIC (in src/.libs/foo.o and src/foo.o, respectively). It's not exactly strange to expect that to work. I admit that I am not sure if it's well-defined that the executables of the project are linked with the shared libs, I must admit. But this does not matter for LAPACK. I am not willing to die on that hill. I can work around the lack of a combined build by building twice and merging the resulting files. So, what about the other changes? |
Speaking about linking executables, if both static and shared libraries are built, then the static library is never tested (in the current setup). |
I will look at them. |
|
I am further scratching my head about properly packaging all the reference lapack. I think I'll open a separate issue about the ILP64 builds and headers. But it would be good to know if my changes here have a chance or not before I create the next PR or push more to the current one. I still like the simultaneous build of shared and static libs, which does properly build with and without PIC, for packaging. But I could work around it if you really oppose the lines I added for that. |
This set of changes stems from the packaging of netlib code in pkgsrc, where we decided to provide blas, lapack, cblas, and lapacke as separate packages and hence need clean support in the build system for this and also for combining the wrappers lapacke and cblas with any prescribed optimized blas or lapack library. - LAPACK option to be able to just build BLAS - consistent logic for USE_OPTIMIZED_BLAS and USE_OPTIMIZED_LAPACK (including clear failure instead of silent fallback if BLAS_LIBARIES or LAPACK_LIBRARIES do not work) - Fix linking of CBLAS on NetBSD with pkgsrc, use Fortran and not C for linking (same as LAPACKE already). - Allow building of shared and static libs at the same time. - Trying to fix up pkg-config files for all cases of internal or external libs The last one is not in really proper shape yet. The idea is that the .pc files contain Libs.private= -L/fooprefix -lfoo if foo is your optimized BLAS/LAPACK, not assuming presence of a .pc file for that itself. This works for user-supplied BLAS_LIBRARIES so far, but inserts the full path to the .so file from its search … which does not fit the case of static linking. And well, yes, you do not know if both static and shared libs are present. So maybe it's a complex problem. My use case so far is fine as I explicitly provide BLAS_LIBRARIES and actually this is what I try to get into other upstream packages that need BLAS or LAPACK: Just use BLAS_LIBS or LAPACK_LIBS as provided by the user (the packager). Would be nice if (most of) this can be included in the official LAPACK CMake build so that we do not have to carry and update the patches all the time.
|
I put some related questions on #461 . Installing separate headers for 32 bit and 64 bit index variant, just like OpenBLAS does, eases packaging, where I build those variants separately. The separation also makes sense since not all platforms support 64 bit integers. The case of a single header for both variants creates a conflict between the packages, or demands a third package just for the headers, in addition to having to specify weird preprocessor flags. Does the project have an opinion on mangling the headers to default to 64 bit indices, like OpenBLAS, or instead ensuring that the .pc and .cmake files contain the proper flags (which they do not right now)? |
|
Static libraries do not contain lists with their dependencies but pkg-config comes in handy here. Example: If a software package A depends on another software package B, the This works because
Here is one of the proposed changes: diff --git a/lapack.pc.in b/lapack.pc.in
index 316c87101..ab5b588bd 100644
--- a/lapack.pc.in
+++ b/lapack.pc.in
@@ -5,5 +5,5 @@ Name: LAPACK
Description: FORTRAN reference implementation of LAPACK Linear Algebra PACKage
Version: @LAPACK_VERSION@
URL: http://www.netlib.org/lapack/
-Libs: -L${libdir} -llapack
-Requires.private: blas
+Libs: -L${libdir} -l@LAPACKLIB@
+Libs.private: @BLAS_PCLIBS@Given a LAPACK build with an external BLAS library selected with Obviously this pull request changes the pkg-config behavior: Instead of recursively looking for static libraries and flags for static linking, the dependency is linked dynamically. Whether the new behavior is right or wrong depends on the user's needs:
|
diff --git a/CBLAS/src/CMakeLists.txt b/CBLAS/src/CMakeLists.txt
index fb1d764c6..445d8f846 100644
--- a/CBLAS/src/CMakeLists.txt
+++ b/CBLAS/src/CMakeLists.txt
@@ -116,7 +116,6 @@ list(REMOVE_DUPLICATES SOURCES)
add_library(${CBLASLIB} ${SOURCES})
set_target_properties(
${CBLASLIB} PROPERTIES
- LINKER_LANGUAGE C
VERSION ${LAPACK_VERSION}
SOVERSION ${LAPACK_MAJOR_VERSION}
)Could you please write a comment or explain why |
| @@ -168,6 +173,18 @@ macro(lapack_install_library lib) | |||
| ) | |||
| endmacro() | |||
|
|
|||
| else() | |||
|
|
|||
| macro(lapack_install_library lib) | |||
There was a problem hiding this comment.
In my opinion it suffices to use a function here.
There was a problem hiding this comment.
Is it advisable to use functions instead of macros? Macro and function will work the same in this case, right?
There was a problem hiding this comment.
I'm out of my league here. I am rather strictly cargo-coding CMake and hope that what works, is also somewhat correct.
| add_subdirectory(SRC) | ||
| else() | ||
| # This is the full path to a .so file from find_package, possibly. | ||
| # not ideal. Better ideas? | ||
| set(LAPACK_PCLIBS ${LAPACK_LIBRARIES}) |
There was a problem hiding this comment.
Given this expression, the pkg-config file will contain a list of paths to libraries. I do not know if this is portable and I would not expect a list of libraries without -l prefix in a pkg-config file. Instead, I would expect pkg-config to return as list of directories and linker flags. That is, instead of
-llapack /usr/lib/x86_64-linux-gnu/libopenblas.so
I expect
-L/usr/lib/x86_64-linux-gnu/openblas-pthread/ -llapack -lopenblas
There was a problem hiding this comment.
I too expect -lfoo instead of a path to an .so file. It is fine in my use-case as I specify build things like this:
LAPACK_COMPONENT= lapacke
LAPACK_COMPONENT_CMAKE_ARGS= \
-DUSE_OPTIMIZED_BLAS=ON \
-DBLAS_LIBRARIES=${BLAS_LIBS:Q} \
-DUSE_OPTIMIZED_LAPACK=ON \
-DLAPACK_LIBRARIES=${LAPACK_LIBS:Q} \
-DCBLAS=OFF -DLAPACKE=ON
# calling cmake with CMAKE_ARGS down the line
This means BLAS_LIBS and LAPACK_LIBS containing the linker flags to get the respective libs (usually the same, but possibly simply -lblas and -llapack, or number funky vendor libs). This is what I want in the .pc file.
I do not know how to tell CMake to construct that from libs it found itself is there a function to translate /path/to/libfoo.so to -L/path/to/libfoo -lfoo? . In my setup, I don't want it to find anything by itself. It should use the libs I tell it to.
The reason for that is also that looking for blas.pc doesn't make sense in a world where a) a .pc (or .cmake) file for the desired BLAS does not exist or b) it is named openblas-xyzprocessor-pthread64.pc or something more weird. For Requires in .pc to make sense, one would have to specify the package name to use during the build. Hardcoding it to 'blas' is just wrong.
Since optimized BLAS libs seem to (optionally) include CBLAS and LAPACKE after all, the combination of these from the netlib build with an optimized BLAS is not on table that often. But I like the idea of having it as building blocks and at least an option of the same level of wrapper API. Eases testing combinations. But this point might become moot once I cave in and build OpenBLAS with CBLAS and LAPACKE directly, while Intel ships it all with MKL in binary anyway.
Edit: But I do think that as long as the libraries are separated as libblas, liblapack, libcblas, liblapacke, it should be possible to build each component by itself, possibly relying on optimized BLAS. The alternative would be a simple build of the whole netlib stack without bothering searching for optimized implementations of parts at all.
By default I am opposed to building shared and static libraries automatically with the same compiler and linker options. In CMake, the immediate effect is code duplication. This problem could be solved by introducing new CMake functions. Next, the compile time is doubled because the codes in shared and static libraries are fundamentally different from each other. Finally, the major problem for me is the value proposition of a simultaneous build. A shared library is for most users on most systems the best choice because of the easy updates and the simplified use because shared libraries contain tables listing their dependencies. There are use cases for static libraries, for example, small executables that do not contain unreachable library code or when code must be cross-compiled. In these scenarios the simultaneous build will not help because the shared library won't be used; special compiler and linker flags might be needed, too. For example, to remove dead code with the GNU linker, every function must be put in its own ELF section by the compiler generating the object files. One can build a shared library with the necessary options but they inhibit certain optimizations (see the GCC documentation of |
Thanks for taking the time. I will answer this one first since it is easy:
|
This patch was introduced with this comment: I am unsure … CBLAS/src does contain C and Fortran sources. Considering that Fortran needs extra runtime, I presume libgfortran would be potentially missing if the BLAS being linked to isn't actually a Fortran library that itself pulls libgfortran in. LAPACKE is all-C, right? I am in the process to changing it, but the initial packaging of BLAS stuff just used BLAS and LAPACK from an optimized (possibly C-based) implementation and put CBLAS from the netlib on top. In this case, CBLAS would be the one needing the Fortran runtime library, not the underlying BLAS, which poses some questions regarding static libs again … but well, you don't need to convince me that nowadays you mostly use shared libs with their recorded dependencies. Any harm caused in linking CBLAS with Fortran? |
Which happens to be named blas.pc in your case, not openblas.pc? I don't see how you can rely on that.
No. It replaces it with the BLAS_LIBRARIES I specified it to use;-) The case of CMake finding it itself and inserting a shared library path is bad and I am welcoming suggestions on how to do this differently. With BLAS stuff, builds searching for an implementation themselves are a nightmare. We possibly have several implementations available (so that HPC users can build their custom software with them), but I ensure that a consistent default one is used for the packaging builds, because otherwise packages nuke themselves out with inconsistent linkage. Guessing is bad here. The BLAS situation is too messy. I have a really hard time explaining it to ‘normal’ computer people. Binary distributions go a differing path, with runtime switching of shared libs, but that is not applicable in my case with pkgsrc trees that offer readily-built software and libraries to build own stuff to differing users. So, how can I phrase this correctly, to have proper linker flags/dependencies in the .pc file for both a located package and me having specified flags to use via BLAS_LIBRARIES? |
Sorry for the late reply to these questions. There is a recent discussion about the need for recursive flags following #486 (comment). DSYEVR was not thread-safe (See https://www.netlib.org/lapack/bug_list.html#_strong_span_class_green_bug112_span_strong_dsyevr_does_not_seem_to_be_thread_safe). Also, DSYEVR wasn't changed since 77a7afb, so it is remained not being thread-safe.
It is certainly better to have multiple build systems working with shared libs options. I think it is a good improvement if these patches can be used in LAPACK. |
You cannot rely on it. For example, with Intel MKL the |
The interference is in the compiler flags if one assumes that static libraries are preferred over shared libraries because of the custom build options that can be used. CMake automatically searches the environment for relevant compiler and linker flags and these are used for all targets. Hence, if the environment variables |
Have you looked at Spack? Spack allows you to have builds of the same software but with dependencies on different implementations and it can create environment module files.
We could let the user pass the name of a pkg-config dependency and optionally its location. If the location is not given, the user could still set the environment variable |
So this is handled in the current build. Good. This flag setting is good. What I don't like is that the build overrides the user-specified FFLAGS. When I say -Os, I would like that to be in effect, not -O2:-( Any specific reason that the user FFLAGS are not appended instead of prepended? If there are nasty flags handed in, that's the user's fault. I don't mind sane defaults in the build itself, but the choice should be honoured. [Edit:
I am not sure if the pkgsrc patches were particularily nice. You can grab them in the pkgsrc CVS (yes, CVS) history, or just in an earlier release tarball. I need to limit the time I spend on this stuff. The actually more interesting thing I need to clear up is to figure out why/if OpenBLAS does not work well with SuiteSparse And I need to install actual software for users. And do my actual job … |
|
I have some comments about the changes related to "consistent logic for USE_OPTIMIZED_BLAS and USE_OPTIMIZED_LAPACK". As I understood, and please correct me if I am wrong, there are at least three use cases where the behavior of the build system will be affected. These are: a) When the user provides USE_OPTIMIZED_BLAS = OFF and BLAS_LIBRARIES=<provided>
b) When the user provides USE_OPTIMIZED_LAPACK = OFF and LAPACK_LIBRARIES=<provided>
c) When the user provides USE_OPTIMIZED_LAPACK = ON and LAPACK_LIBRARIES=<provided>
I don't like the new behaviors for (a) and for (b). I prefer either the current one a FATAL message terminating the build. It seems odd to just ignore the input. I like (c) :) |
Exactly my point. The established way to tell about a BLAS implementation (especially BLAS, perhaps less so for CBLAS if one wants to find the header, too) is to hand in the needed linker flags, e.g. The ideal handling would be one of
I see pkg-config handling for BLAS stuff really as seconday. Generally, I like .pc files being around and rely on them for my own software. But the BLAS situation is special, where you simply don't know which .pc file to choose without asking the user's preference. Also details like ILP64 or not. It's just messed up as it is and glossing over pkg-config doesn't suddenly make it simple. So, what do you want from this PR. Should I test if BLAS_LIBRARIES is user-specified and use that for BLAS_PCLIBS, otherwise keep the current state and you later fix it up to work for pkg-config module selections if so desired? And set PC_LAPACK_PRIVATE to either the correct |
|
Maybe we should split your contributions into different PRs to ease discussion with the community. "LAPACK option to be able to just build BLAS" and "consistent logic for USE_OPTIMIZED_BLAS and USE_OPTIMIZED_LAPACK", for instance, are orthogonal to the use of |
This set of changes stems from the packaging of netlib code in pkgsrc, where we decided to provide blas, lapack, cblas, and lapacke as separate packages and hence need clean support in the build system for this and also for combining the wrappers lapacke and cblas with any prescribed optimized blas or lapack library. - LAPACK option to be able to just build BLAS - consistent logic for USE_OPTIMIZED_BLAS and USE_OPTIMIZED_LAPACK (including clear failure instead of silent fallback if BLAS_LIBARIES or LAPACK_LIBRARIES do not work) - Fix linking of CBLAS on NetBSD with pkgsrc, use Fortran and not C for linking (as there is Fortran code as part of it, depending on the runtime). - Allow building of shared and static libs at the same time. - Trying to fix up pkg-config files for all cases of internal or external libs The last one is not in really proper shape yet. The idea is that the .pc files contain Libs.private= -L/fooprefix -lfoo if foo is your optimized BLAS/LAPACK, not assuming presence of a .pc file for that itself. This works for user-supplied BLAS_LIBRARIES so far, but inserts the full path to the .so file from its search … which does not fit the case of static linking. And well, yes, you do not know if both static and shared libs are present. So maybe it's a complex problem. My use case so far is fine as I explicitly provide BLAS_LIBRARIES and actually this is what I try to get into other upstream packages that need BLAS or LAPACK: Just use BLAS_LIBS or LAPACK_LIBS as provided by the user (the packager). Would be nice if (most of) this can be included in the official LAPACK CMake build so that we do not have to carry and update the patches all the time.
I have no strong opinion on what is the best option for linking C with Fortran code. What I do know is that some users have problems in installing libgfortran on MacOS, and a possible solution is to use gfortran to link. |
|
Certainly, Christoph and @drhpc have more experience with that than I. I would just like to remark that this PR keeps the same behavior from before when the new flag BUILD_STATIC_LIBS is OFF. Moreover, I believe most of the users wouldn't use this flag unless they know what they are doing since:
I like the way you designed it, and the idea of having a new feature. However, I think this shall be well documented in the manual, or at least in the CMakelists. Something like:
What do you think? |
|
About both targets and flags:
As @weslleyspereira points out, the default is still to build only one varirant. If the user specifies something like The only real difference is adding of PIC or not, and that is correctly done. Really, the only issue I see is that you might not want those few duplicated lines in the CMakeLists.txt. About documentation: Sure, actually I think it should be a declared option, and there it can be briefly explained what it does. I pushed to the messy branch again (I'm too busy/fed up to fix up the commit history, hope the changes are still clear) with it being a documented option. OK? PS: If we could start to settle on things, I have some hope that we could get it done with this PR. I must admit that I am really worn out because this is a locally solved problem that takes up too much of my time now, just because I 'want to make it right' by merging it upstream. I have lots of other builds fix. PPS: About Spack. Yes I know it. It is a solution aimed at the same field of application, but solves a somewhat different problem than what I do with pkgsrc. And pkgsrc is not just for that but it's the userspace layer for NetBSD, which also works on other unixy systems. So it has the requirements of a regular distro, like (at least for some libs) providing shared and static libs for developers. |
I see that this could help … but I also have the feeling that we could get to some conclusions here. If the end is that the discussed changes should come in a series of PRs that are handled in order, I could manage to create those, I guess. The last one to settle and further discuss is probably the .pc file thing, as my solution there is far from complete yet. But this is also one where I could use help. I have low incentive to fix up a variant that does not use BLAS_LIBRARIES in Libs.private as that is my only use-case. So if you can apply the agreed changes to handle perhaps both specified libs and linker flags and another pkg as Requires yourself without me reiterating it in a PR, that would be highly appreciated. |
| @@ -105,3 +105,11 @@ set_target_properties( | |||
| SOVERSION ${LAPACK_MAJOR_VERSION} | |||
| ) | |||
| lapack_install_library(${BLASLIB}) | |||
| if(BUILD_SHARED_LIBS AND BUILD_STATIC_LIBS) | |||
| add_library(${BLASLIB}_static STATIC ${SOURCES}) | |||
There was a problem hiding this comment.
Does lapack export those targets as -config.cmake? If it does please provide a non-suffixed alias which can be switched between static/shared
There was a problem hiding this comment.
I don't know enough CMake to follow through on that. An alias for what exactly? I must admit that I'd like a world (on Unix-y systems, yes) with more pkgconfig files and less specific CMake build config files. I have no contact to those if I can help it:-/
There was a problem hiding this comment.
with more pkgconfig files
proper pkgconfig is difficult and mixing shared/static is in my eyes bad. vcpkg has corrective measures to fix pc files depending of the specified lib linkage and als makes them relocatable by using ${pcfiledir}
As far as i can tell lapack exports its targets since this change only affects builds of both libraries I don't care that much since vcpkg always builds either shared or static.
I have no contact to those if I can help it
People should just have a consistent interface in using lapack targets. As long as the single lib build is not affect this is not an issue. Only if people have both libs they need to decide to use lapack_static or lapack which is annoying for downstream users. as such you would normally add an imported interface targets (not an alias because those are sometimes problematic) which depending on some user setting switches between lapack_static and lapack_shared.
There was a problem hiding this comment.
I second my comment below about rather getting rid of this Cmake-specific dependency mechanism. But, after all this time, I feel indeed that I could make my life easier by dropping everything about static libraries here. I wanted to support them out of tradition. They used to be provided as a default. But if we have discussions spanning years about building them alongside or not … maybe it's finally time to get rid of them also in our system-wide installs. I could re-visit my branch and rip out the static-and-shared-at-the-same-time stuff and see if the other changes could be massaged to be acceptable. I'd need to convince folks over at pkgsrc that nobody really needs static libs anymore. I guess that should work.
| if(USE_OPTIMIZED_BLAS) | ||
| if(BLAS_LIBRARIES) | ||
| include(CheckFortranFunctionExists) | ||
| set(CMAKE_REQUIRED_LIBRARIES ${BLAS_LIBRARIES}) |
There was a problem hiding this comment.
please append. set overwrites user supplied values.
There was a problem hiding this comment.
So
append(CMAKE_REQUIRED_LIBRARIES ${BLAS_LIBRARIES})
?
There was a problem hiding this comment.
I don'T know if the variable is a string or list it is either.
list(APPEND <listvar> "<value>")
or string(APPEND <stringvar> "<value>")
There was a problem hiding this comment.
OK, thanks. I'll try to look into it next time I free a time slot in my brain for this mess. At the moment I toy with the idea of getting out of all that packaging business. I could actually write programs that do something … maybe that impulse will pass …
| if(BLAS_FOUND) | ||
| message(STATUS "--> BLAS supplied by user is WORKING, will use ${BLAS_LIBRARIES}.") | ||
| if(USE_OPTIMIZED_BLAS) | ||
| if(BLAS_LIBRARIES) |
There was a problem hiding this comment.
In general: I think this if shouldn't exists. If the user supplied values trust him/her. It is not the responsible of the build description to check valid inputs.
There was a problem hiding this comment.
I'm following you on that … I just extended existing logic, not changing everything. My dream is to specify BLAS_LIBS and have them just used or alternatively the name of a pkg-config package for the same purpose. Actually, I have CMake FindBLAS modifications pending to be able to override the automagic there with a provided pkg-config package name.
There was a problem hiding this comment.
My dream is to specify BLAS_LIBS and have them just used or alternatively the name of a pkg-config package for the same purpose
You can always provide a custom FindBLAS.cmake via CMAKE_MODULE_PATH where you can do this (if you adhere to variables defined by the inbuild CMake module ). Build descriptions should stay as clean as possible from library logic. This also means using targets instead of library names: e.g. linking against BLAS::BLAS (probably not possible due to cmake restrictions? lapack could provide its own module for it. )
There was a problem hiding this comment.
Well, yes … or patch the upstream one in the build. My goal is to have such stuff upstreamed instead of hacking around it locally. Actually, my whole point is to get rid of the automagic with hardcoded lists in FindBLAS.cmake altogether. If I have to extend it to achieve that, be it that way. It irks me to have a build-system-specific dependency mechanism that duplicates the efforts of pkg-config.
There was a problem hiding this comment.
automagic with hardcoded lists in FindBLAS.cmake
I hope your hardcoded list takes care of custom suffixes and prefixes ;)
| Cflags: -I${includedir} | ||
| Requires.private: @BLASLIB@ |
There was a problem hiding this comment.
Requires is better than Libs.private since it allows to inject a dependent pc file.
There was a problem hiding this comment.
For example. vcpkg builds lapack-reference with openblas. openblas installs a openblas.pc file. In vcpkg there is a metaport called blas which supposly selects the blas implementation. As such this metaport install a blas.pc files which simply has a Requires(.private): openblas.
There was a problem hiding this comment.
But with Requires you're stuck with pkg-config files. In a perfect world, you'd have .pc files for each installed BLAS and configuration of it. I'm working on that (although I see, from dealing with this year-old attempt at this stuff, that my motivation is waning towards more productive endeavours). But right now, the common denominator is a plain list of linker arguments, which you can easily provide using pkg-config in the cmake command line anyway.
You mention one way to deal with this mess by introducing a metaport that installs a redirecting blas.pc. Another layer of confusion … my world installs differing BLAS implementations at the same time and, while there will be a default one, not everyone is pointed to it. While this is a viable approach as such, we have the ambiguity here that the name blas.pc is traditionally taken by Netlib's implementation. If that is a placeholder, no actual imeplementation should install it.
There might be a discussion here … but in the context of a packaging system, I don't see it, really. If you can put in
Requires.private: openblas
you can also expand that to what it would do, or even just copy the file. Please understand that I'm not fighting the idea of having more things set up with .pc files, but that my current view is that things should work if the BLAS implementation doesn't offer one (or a broken one). BLAS doesn't usually depend on a lot of other libraries (well, usually), so the added value of a .pc file is limited. Just Libs is the more general solution.
There was a problem hiding this comment.
you're stuck with pkg-config files
In a PC file you are already stuck with pkg-config.
blas.pc is traditionally taken by Netlib's implementation
I plan to renamed that to blas-reference.pc in vcpkg. blas is just a concept just like ssl. It doesn't directly define a certain implementation.
but that my current view is that things should work if the BLAS implementation doesn't offer one
According to your world view you could just provide/fix one if you need depend on it. I don't really get why you are adding the -l flags here. You are pro pkg-config so why use a hack which breaks the principles of responsibility? cblas.pc file is not responsible for defining the -l flags to link blas. That is the responsibility of blas.pc.
you might also not be aware of the fact that BLAS_LIBRARIES might contain generators expressions or keywords like debug/optimized. Don't think this is relevant for this specific cblas.pc case but it might be relevant for another change withing this PR.
BLAS doesn't usually depend on a lot of other libraries
Unless you use blas from lapack on windows and need to link in the fortran runtime libraries which are not automatically available using MSVC.
There was a problem hiding this comment.
I'm leaning towards using the name netlib instead of reference. Somehow it seems to be more proper (traditional?).
About fixing up pkg-config files, I am actually intending to do that, including preparing one for Intel MKL stuff as used in the pkgsrc prefix. It's just that we have to live in a world with transitions. I'll focus now on getting the CMake BLAS stuff recognize given pkg-config files. Consequently, the netlib reference build should also just use pkg-config, in that world … I just have to ponder a bit about actually ditching the tradition of providing BLAS_LIBS and LAPACK_LIBS with the actual linker arguments to use.
| message(STATUS "Using supplied NETLIB BLAS implementation") | ||
| add_subdirectory(BLAS) | ||
| set(BLAS_LIBRARIES ${BLASLIB}) | ||
| set(BLAS_PCLIBS -l${BLASLIB}) |
There was a problem hiding this comment.
Care to elaborate? Is this the same point about Libs and Requires above?
There was a problem hiding this comment.
It is the same point as above. Don't add libs of other pc files into an unrelated pc file. Rather directly depend on the pc file in Requires.
|
So I just reviewed the discussion after deleting the repo two weeks ago. I didn't remember it still containing unmerged things. Well, clearly, it didn't contain a state ready to be merged. Maybe I'll come back with smaller changes once I decide on bringing the packaging situation forward. Or maybe not. I have to re-evaluate my approach to investing my time into packaging other people's software. Specifically, how much time I want to spend with CMake. |
This set of changes stems from the packaging of netlib code in pkgsrc,
where we decided to provide blas, lapack, cblas, and lapacke as separate
packages and hence need clean support in the build system for this and
also for combining the wrappers lapacke and cblas with any prescribed
optimized blas or lapack library.
(including clear failure instead of silent fallback if BLAS_LIBARIES
or LAPACK_LIBRARIES do not work)
for linking (same as LAPACKE already).
external libs
The last one is not in really proper shape yet. The idea is that the
.pc files contain
Libs.private= -L/fooprefix -lfoo
if foo is your optimized BLAS/LAPACK, not assuming presence of a .pc file
for that itself. This works for user-supplied BLAS_LIBRARIES so far, but
inserts the full path to the .so file from its search … which does not fit
the case of static linking. And well, yes, you do not know if both static
and shared libs are present. So maybe it's a complex problem. My use case
so far is fine as I explicitly provide BLAS_LIBRARIES and actually this is
what I try to get into other upstream packages that need BLAS or LAPACK:
Just use BLAS_LIBS or LAPACK_LIBS as provided by the user (the packager).
Would be nice if (most of) this can be included in the official LAPACK CMake
build so that we do not have to carry and update the patches all the time.
Description
Checklist