new macro to check and test for lib and header (#143)

svigerske commented 3 years ago

Adds macro

AC_COIN_CHK_LIBHDR([prim],[clients],[lflgs],[cflgs],[dflgs],
              [function-body],[includes],
              [dfltaction],[cmdopts])

and replaces AC_COIN_CHK_LIB by a wrapper around AC_COIN_CHK_LIBHDR.

LouHafer commented 3 years ago

@svigerske, I've ended up doing a fair bit of tweaking to CHK_LAPACK, to better handle configure command line parameters. I can kind of see a general problem coming where lines of code that are commented on here are going to disappear. What's the preferred strategy? Just go ahead an push changes into the issue143 branch?

svigerske commented 3 years ago

@svigerske, I've ended up doing a fair bit of tweaking to CHK_LAPACK, to better handle configure command line parameters. I can kind of see a general problem coming where lines of code that are commented on here are going to disappear. What's the preferred strategy? Just go ahead an push changes into the issue143 branch?

Yes, that should be no problem. GitHub should recognize that the comments are for an older version and still show the correct diff (at least that's how it works in GitLab).

LouHafer commented 3 years ago

Well piffle! Apologies for polluting the log with all those past commits. I'm still baffled by git some days. Looked much cleaner locally, a nice fast-forward with four commits. Anyway, I'm hoping this addresses all the issues except the question of splitting PRIM_CHK_HDR. I'll consider that one after lunch.

tkralphs commented 3 years ago

Before we merge this, let's clean up the history a bit. Lou, I guess you are pulling master with the default merging behavior. You need to pull with rebase in order to avoid all of those old commits coming in. You will need to do a force push to your fork after rebasing, but this shouldn't cause any issues for branches where you are the only contributor.

(You can make pulling with rebase your default with git config --global pull.rebase true, which Is a good idea in many cases)

LouHafer commented 3 years ago

@svigerske, here's the mass of changes I mentioned in email. I've pulled COIN_CHK_PKG and COIN_FIND_PRIM_PKG into a separate file for ease of working and rewritten them to conform to the style and algorithms of the LIBHDR chain. dfltaction = build will work for simple cases in the PKG chain, and there's now a big ugly macro, COIN_DEPRECATE_BUILD_OPTION, in coin_chk_libhdr.m4 that will force a fatal error if the LIBHDR chain sees 'build' and issue a warning if the PKG chain sees 'build'. This happens at run_autotools time --- only the developer can do anything to fix the problem. There are a bunch of other smaller changes. Have a look and try them on your favourite project and let me know what breaks, or what you don't much care for.

LouHafer commented 3 years ago

@svigerske, doing a drive-by check and noticed the `thumbs up'. Let me know when (if) you think we're ready to try and merge this and I'll do up a clean pull request. Given the changes, we may well see the occasional bit of breakage in existing configure.ac and Makefile.am. If there are any projects you think might be at risk, let me know. I should have the odd day over the next few weeks and can try to test.

svigerske commented 3 years ago

I think the changes are ready to be tried. Not sure I am ready, too. We shouldn't merge before trying them. We can do this in separate branches of your and my favorite projects to see what Teds travis and appveyor tests are saying.

CLAassistant commented 3 years ago

All committers have signed the CLA.

svigerske commented 3 years ago

I tried AC_COIN_CHK_LIBHDR in this form:

AC_LANG_PUSH(C++)
AC_COIN_CHK_LIBHDR(SoPlex,[OsiSpxLib OsiTest],[],[],[],
  [new soplex::DIdxSet()],
  [#include "soplex.h"
   #if SOPLEX_VERSION >= 400
   #error "OsiSpx requires SoPlex < 4.0"
   #endif
  ],
  [default_skip])
AC_LANG_POP(C++)

and it works when I specify no flags for SoPlex, flags for SoPlex 3.1.1, and flags for current SoPlex for configure. In the latter case, "works" means that I get

checking for library SoPlex with combined link and compile check... no (header compile, link with header)
configure: WARNING:
Compiler flags are "-I/path/to/soplex/src". If you expected this test to
succeed, check that they are correct. You can supply correct values using
--with-soplex-cflags.
configure: WARNING:
Linker flags are "-L/path/to/soplex/lib -lsoplex". If you expected this test to
succeed, check that they are correct. You can supply correct values using
--with-soplex-lflags.
configure: WARNING:
Check config.log for details of failed compile or link attempts.

(I find all these linebreaks too much, but ok).

My concern is that configure keeps running, finishing with

configure: Configuration of Osi successful

I think it should stop with an error if the flags that the user provided explicitly do not work.

svigerske commented 3 years ago

On Cbc, there is

AC_COIN_CHK_PKG([Nauty],[CbcLib CbcSolverLib],[nauty])

I configure with with_nauty_lflags=-lnauty, which are working flags.

But the macro seems to record nauty as a .pc file that should be used for CbcLib, etc, i.e.,

CBCLIB_PCFILES='osi-cplex nauty osi-clp clp cgl osi coinutils '

and this gives me a number of warnings from the finalize_flags part in configure:

Package nauty was not found in the pkg-config search path.
Perhaps you should add the directory containing `nauty.pc'
to the PKG_CONFIG_PATH environment variable
Package 'nauty', required by 'virtual:world', not found
Package nauty was not found in the pkg-config search path.
Perhaps you should add the directory containing `nauty.pc'
to the PKG_CONFIG_PATH environment variable
Package 'nauty', required by 'virtual:world', not found
Package nauty was not found in the pkg-config search path.
Perhaps you should add the directory containing `nauty.pc'
to the PKG_CONFIG_PATH environment variable
Package 'nauty', required by 'virtual:world', not found
Package nauty was not found in the pkg-config search path.
Perhaps you should add the directory containing `nauty.pc'
to the PKG_CONFIG_PATH environment variable
Package 'nauty', required by 'virtual:world', not found

(and linking fails)

If a user specifies some --with-xyz-* flag, then none of the defaults passed to AC_COIN_CHK_PKG should be used, I believe.

svigerske commented 3 years ago

@svigerske, doing a drive-by check and noticed the `thumbs up'. Let me know when (if) you think we're ready to try and merge this and I'll do up a clean pull request. Given the changes, we may well see the occasional bit of breakage in existing configure.ac and Makefile.am. If there are any projects you think might be at risk, let me know. I should have the odd day over the next few weeks and can try to test.

I've now tried this on Ipopt, CoinUtils, Osi, and Cbc in branches configure-update. In the configure.ac, I removed the use of [build] and replaced AC_COIN_CHK_LIB by AC_COIN_CHK_LIBHDR.
That works on my machine now (if I turn off nauty, see above). travis/appveyor builds can be found at

https://travis-ci.com/github/coin-or/Ipopt
https://ci.appveyor.com/project/coin-or/ipopt
https://travis-ci.com/github/coin-or/CoinUtils
https://ci.appveyor.com/project/coin-or/coinutils
https://travis-ci.com/github/coin-or/Osi
https://ci.appveyor.com/project/coin-or/osi
https://travis-ci.com/github/coin-or/Cbc
https://ci.appveyor.com/project/coin-or/cbc (make sure to look at the build for the configure-update branch)

We also need to update the docu in docs/configure.md before merging.

LouHafer commented 3 years ago

@svigerske, I've made a pair of small changes as suggested in your comments. CHK_LIBHDR issues the same warning message as before, then forces an error stop if the failure results from user-supplied flags. FIND_PRIM_PKG now clears prim_pcfiles if the user supplies any flags. See if this does it for you.

LouHafer commented 3 years ago

@svigerske, I hate to bring up a new glitch, but as I got to rewriting configure.md I ended up looking at COIN_CHK_HERE, and now I'm looking at commit https://github.com/coin-or-tools/BuildTools/commit/5fae46779a7349c285646e1267579470dfae828c. Seems to me the check

if test -n "$m4_toupper(myvar)_PCFILES"

is the wrong check, it should consider m4_default($3, m4_tolower($1)), which is the name of the .pc file being added. More to the point, this can never be null, so there's no reason to check at all.

Which got me to thinking, 'What if I want to use CHK_HERE for a primary that doesn't get installed? No reason that it should have a .pc file, it's not useful.' So maybe CHK_HERE really does need to consider the possibility that there is no .pc file. The change would not be backward compatible. There are surely places where the default is used. Osi2, for one. Still, if we're going to change, this is as good a time as any.

LouHafer commented 3 years ago

So fixing up the documentation is a good thing. https://github.com/coin-or-tools/BuildTools/pull/148/commits/1f157211e0b91e09594d134e5fef35824e3d9747 because I thought maybe it'd be good to actually check that COIN_SKIP_PROJECTS works :-)

LouHafer commented 3 years ago

@svigerske, I've done a major rewrite of configure.md. See what you think.

svigerske commented 3 years ago

@svigerske, I hate to bring up a new glitch, but as I got to rewriting configure.md I ended up looking at COIN_CHK_HERE, and now I'm looking at commit 5fae467. Seems to me the check
if test -n "$m4_toupper(myvar)_PCFILES" 
is the wrong check, it should consider m4_default($3, m4_tolower($1)), which is the name of the .pc file being added. More to the point, this can never be null, so there's no reason to check at all.

Yes, right. I fixed that in this branch now (67267ca). AC_COIN_CHK_HERE was not much used and the other two places from 5fae467 were ok.

Which got me to thinking, 'What if I want to use CHK_HERE for a primary that doesn't get installed? No reason that it should have a .pc file, it's not useful.' So maybe CHK_HERE really does need to consider the possibility that there is no .pc file. The change would not be backward compatible. There are surely places where the default is used. Osi2, for one. Still, if we're going to change, this is as good a time as any.

Yes, would be ok to change this now.

svigerske commented 3 years ago

@svigerske, I've done a major rewrite of configure.md. See what you think.

Looks very good. Thank you!

svigerske commented 3 years ago

So, to me, this looks good now. My machine and travis+appveyor were ok on the corresponding branches of Ipopt, CoinUtils, Osi, and Cbc.

@LouHafer Can we just merge this, or you want to work a bit more on AC_COIN_CHK_HERE first?

LouHafer commented 3 years ago

@svigerske, it's true that the issue with COIN_CHK_HERE is a solution in search of a problem. It can be dealt with later if anyone complains. Is it easy for you to do a clean pull from this branch in spite of the excess history I dragged in? If not, say so and I'll cobble together a clean pull request.

svigerske commented 3 years ago

@svigerske, it's true that the issue with COIN_CHK_HERE is a solution in search of a problem. It can be dealt with later if anyone complains. Is it easy for you to do a clean pull from this branch in spite of the excess history I dragged in? If not, say so and I'll cobble together a clean pull request.

So I did a git rebase -i master and removed commits from the past that had already been cherry-picked into master or were shown twice (probably because of this merging back-and-forth). That led me rebase with almost no conflicts and the final result looks the same (empty diff).

coin-or-tools / BuildTools

new macro to check and test for lib and header (#143) #148