coin-or-tools / BuildTools

Macros and patches for GNU autotools
https://coin-or-tools.github.io/BuildTools/
Other
3 stars 7 forks source link

Proper checks for linking and header presence and compilation #143

Closed LouHafer closed 3 years ago

LouHafer commented 3 years ago

This started out as Osi pull request https://github.com/coin-or/Osi/pull/144, which originated from https://github.com/coin-or/Osi/issues/143. At the point of transfer, we were evolving towards a new macro of this form:

COIN_CHK_LIBHDR([prim],[client packages],[lflgs],[cflgs],[dflgs],
    [function-body],[includes],
    [dfltaction],[cmdopts])

This is flexible enough to handle checks for C++ overloaded methods and it's a good match to the underlying autoconf macro construct AC_LINK_IFELSE([AC_LANG_PROGRAM(...)],[],[]) that we should be using instead of the obsolete AC_TRY family. Keeping the function-body and includes separate will make it possible to provide better diagnostics (link failure vs. include failure). The drawback is that asking the user to supply 'function-body' puts more of a burden on the user, particularly for the up-to-now common case where the user just gives a function name and trusts autoconf to construct the required program fragment. But we can hide this (and avoid a whole lot of backward compatibility problems) if we make the existing COIN_CHK_LIB macro

COIN_CHK_LIB([prim],[client packages],[lflgs],[cflgs],[dflgs],
    [func],[other libraries],
    [dfltaction],[cmdopts])

into a wrapper for COIN_CHK_LIBHDR. We construct the trivial function-body from func, just as autoconf does now. Taking a bit more risk, it seems that the other libraries parameter is mostly (universally?) unused. We could repurpose it to header file.

Stefan, this doesn't quite capture your idea of providing a whole program, and I didn't quite follow what you had in mind as 'compile time version check'. Do you have some examples where keeping function-body and includes separate would cause a problem?

svigerske commented 3 years ago

Keeping function-body and includes separate should be fine. That is both just C/C++ code (if testing C/C++), right?, some will be added before the int main(), the other within the int main() { ... }.

With version check I mean adding something like

#if CPX_VERSION < 800
#error "CPLEX too old"
#endif

That could go into either function-body or includes then and would make the test fail if CPLEX is there but too old.

LouHafer commented 3 years ago

So I hacked together a preliminary implementation. Seemed to me some simplification of the algorithm in FIND_PRIM_LIB was in order, so I'm working on that. I also hacked out the 'build' option for dfltaction --- it only works for ThirdParty and folks should really use CHK_PKG for that. I'm using an install of MKL per Intel's directory layout but off in a nonstandard place as a test case. Here's what it looks like, with the old CHK_LIB, and the new CHK_LIBHDR.

MKLROOT=/devel/InstRoot/Intel/mkl

AC_COIN_CHK_LIB(MKL,MklTest,
  [-L${MKLROOT}/lib/intel64 -lmkl_intel_ilp64 -lmkl_sequential -lmkl_core],
  [],[],[dgemm])

AC_COIN_CHK_LIBHDR(MKL2,Mkl2Test,
  [-L${MKLROOT}/lib/intel64 -lmkl_intel_ilp64 -lmkl_sequential -lmkl_core],
  [-DMKL_ILP64 -m64 -I${MKLROOT}/include],[],
  [const char *transa, *transb ;
   const double *dummy1, *dummy2, *dummy3, *dummy4 ;
   double *dummy5 ;
   const MKL_INT *n, *m, *k, *l, *p, *q ;
   dgemm(transa,transb,n,m,k,dummy1,dummy2,l,dummy3,p,dummy4,dummy5,q) ;],
  [#include "mkl.h"])

CHK_LIB does only the link check. CHK_LIBHDR does both a compile and link check and includes the header in the link check. I haven't gotten to the point of making CHK_LIB a wrapper for CHK_LIBHDR. I think it'll work but it'll be necessary to separate the header compile and function link checks. No way to guess the mess required for a proper C++ declaration. Enough for today.

LouHafer commented 3 years ago

@svigerske, I've committed some code over in my repository. Have a look when you have a moment and see what you think.

There's a new branch of BuildTools called Issue143. The file coin_chk_libhdr.m4 contains new macros that will do both header compile and function link checks. There's also a repository, Testbed, with a configure.ac that shows a bunch of variations on how to use them. (You'll want to tweak the macro calls to fit your system.)

I did a build of the short optimisation stack (CoinUtils -- Cbc) and the wrapper version of CHK_LIB didn't cause problems. But if anyone actually used the old [other libraries] parameter there will be problems. CHK_LIB_HDR offers considerable additional flexibility. The old CHK_LIB is renamed CHK_LIB_OBS in coin.m4.

Semantics have changed for command line configure flags. You can override one of --with-prim-lflags or --with-prim-cflags without affecting the other. I think the whole algorithm flow in FIND_PRIM_LIB is now more clear: set defaults at the top and override as necessary. Looking at it, I can't think why I wrote such a convoluted algorithm initially. Must have been a 'just grew' kind of thing. The semantics of skipped, yes, no have also changed slightly. On the down side, the DFLT_HACK macro is unsightly but accomplishes the goal of staying within the 9 parameter limit for POSIX m4. And some care is required to get macro parameters expanded at the right moment.

Note the asymmetry in the use of LANG_SOURCE for separate link check vs. LANG_PROG when the header and function body are compiled together.

The usual caveat applies: my testing environment is limited to Fedora, so I've probably missed some problems.

svigerske commented 3 years ago

I copied your commits into a branch in this project, so I can modify. Also made a pull request (#148), so we can comment.

But looked good from a quick look. I think we just have to go through some projects and update their configure.ac to use the new macro (in a separate branch). Then Ted's CI will run this on windows and macOS, too.

svigerske commented 3 years ago

Fixed by #148.

tkralphs commented 3 years ago

Thanks, gentlemen!