configure: strip -L/usr/lib and similar from netCDF, MySQL, GEOS and SFCGAL lib path

[eleaver]

Addresses bug #2357: In gdal/configure geos and netcdf library paths shadow local --with-proj. This PR changes configure.ac to strip explicit leading -L/usr/lib or -L/usr/lib64 from GEOS_LIBS and NETCDF_LIBS. Reran "autoconf configure.ac > configure" to update "configure" as well. BUT PLEASE NOTE my autoconf is version 2.69, NOT 2.70, therefore the new --runstatedir option introduced with autoconf-2.70 does not appear in the present configure. If this is an issue, either reject this PR, re-run autoconf-2.70 on the merged result if autoconf-2.70 is available, or otherwise make sure git merge retained the --runstatedir option in the merged result.

What does this PR do? (see above)

What are related issues/pull requests? bug #2357

Tasklist

I wished to use Gdal-3.0.4 and later on Fedora30, whose system proj package is 5.2.0 whereas gdal-3.x.y needs proj 6. So I compile and install /usr/local/proj-6.3.1 on both Fedora30 AND Fedora32 (beta), whose system proj is also 6.3.1, and verified that gdal autotests give identical results on Fedora32 when gdal is configured with either the system proj-6.3.1 or with the /usr/local/proj-6.3.1. So that part looks good.

Unfortunately, the autotest results on Fedora30 are not identical with those on Fedora32:

============================= test session starts ==============================
Fedora30 (up-to-date)
platform linux -- Python 3.7.6, pytest-3.10.1, py-1.7.0, pluggy-0.8.1
rootdir: /home/leaver/project/Grass/Requisites/gdal/git/GDAL/autotest, inifile: pytest.ini
plugins: sugar-0.9.2, env-0.6.2
collected 7064 items
71 failed, 5689 passed, 1302 skipped, 2 xfailed, 39 warnings in 1864.04 seconds
============================= test session starts ==============================
Fedora 32 (beta 1.2 up-to-date)
platform linux -- Python 3.8.2, pytest-4.6.9, py-1.8.0, pluggy-0.13.0
rootdir: /home/leaver/project/Grass/Requisites/gdal/git/GDAL/autotest, inifile: pytest.ini, testpaths: ogr, gcore, gdrivers, osr, alg, gnm, utilities, pyscripts
plugins: sugar-0.9.2, env-0.6.2
collected 7064 items
= 56 failed, 5636 passed, 1370 skipped, 2 xfailed, 22 warnings in 1390.74 seconds =
==========================================================================

Fedora32 fails fifteen fewer than Fedora30. It is not clear to me why. Any insight will be welcome. Thanks!

• ADD YOUR TASKS HERE
• Add test case(s)
• Review
• Adjust for comments
• All CI builds and checks have passed

Environment

Provide environment details, if relevant:

• OS: Fedora 30 and 32 on i2500K with Nvidia GTX960. Gdal configured --with-opencl and python3. (I can post my ConfigureGDAL.sh script if desired.)
• Compilers: gcc-9.3.1 (fc30) and gcc-10.0.1 (beta fc32)

[rouault]

This might be insufficient for other OS. For example on Ubuntu

$ geos-config --libs
-L/usr/lib/x86_64-linux-gnu -lgeos-3.5.0

[eleaver]

Thank you Even, it will indeed be insufficient on Ubuntu if |/usr/lib/x86_64-linux-gnu| is their system 64-bit path equivalent to RedHat's /usr/lib64. But it should be easy enough to add. I've a spare partition for Ubuntu, and am downloading 18.04 now. I'll try to test it over the weekend, and perhaps suggest some addenda to autotest/README.md as well (in a new documentation issue, not the present #2395). A question arises how best to add this filter. As is, I strip -L/usr/lib or -L/usr/lib64 only if those specific strings appear in GEOS_LIBS or NETCDF_LIBS. It will be easy to add -L/usr/lib/x86_64-linux-gnu as well. But the problem might multiply with other OS. Alternatively, one might wildly strip all -L/usr/lib64, -L/usr/lib64/* -L/usr/lib, -L/usr/lib/*, and hope someone somewhere has does not install a custom libproj -- or whatever -- in one of those locations rather than a subfolder of /usr/local or /opt where they belong. One can save the world from only so much, but it's worth considering.

…

On 4/10/20 3:21 AM, Even Rouault wrote: This might be insufficient for other OS. For example on Ubuntu |$ geos-config --libs -L/usr/lib/x86_64-linux-gnu -lgeos-3.5.0 | — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2395 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACXGYWKRBOT5OIP53AYMH7DRL3XKLANCNFSM4MFBQ27A>.

[rouault]

Alternatively, one might wildly strip all -L/usr/lib64, -L/usr/lib64/* -L/usr/lib, -L/usr/lib/*

That seams reasonable to me

[rouault]

At least for consistence with the rest of the code:

Suggested change

	elif test x$with_proj != x/usr -a x$with_proj != x/ ; then
	elif test "x$with_proj" != "x/usr" -a "x$with_proj" != "x/" ; then

[rouault]

At least for consistence with similar practice in that file

Suggested change

	proj_libdir=$with_proj/lib
	proj_libdir="$with_proj/lib"

(to adapt in similar cases)

[rouault]

is this change really needed for this pull request ? I don't see the connection. I'd prefer to see it go through another one if possible
In ogr/ogrsf_frmts/geojson/libjson/GNUmakefile, we already have:

# -D_XOPEN_SOURCE=500 to enable strdup() definition in C11 mode
CPPFLAGS := -D_XOPEN_SOURCE=500 $(CPPFLAGS)

[eleaver]

At this point I believe it is necessary. But I don't know why. Last night I was seeing a "strdup undefined" error on OSX, with ld suggesting we #include <string.h>. But json_object.c does #include <string.h>. The problem appears to be that string.h on OSX defines strdup() only if _XOPEN_SOURCE >= 700L. And by default autoconf sets _XOPEN_SOURCE = 500L. So OSX strdup is undefined. Two obvious questions:

1. Why did it fail last night, and not before? Did my (seemingly) unrelated changes to configure.ac trigger an _XOPEN_SOURCE change? git should know; I'll have to ask.
2. If _XOPEN_SOURCE = 700L is indeed required, would it not be better to set it once in configure.ac, rather than in json_object.c, json_tokener.c etc.?
   Ref: https://lists.gnupg.org/pipermail/gnupg-devel/2017-February/032550.html

Is there a way to tag this PR a WIP (Work In Progress)? I'm still working on my new STRIP_SYSTEM_LIBRARY_PATHS macro, and it's likely it might pass all COIN build tests before we're both satisfied with it, and I wouldn't want to see it prematurely merged. Thanks!

[eleaver]

Okay. So configure sets #define _XOPEN_SOURCE 700; and ogr/ogrsf_frmts/geojson/libjson/GNUmakefile sets CPPFLAGS := -D_XOPEN_SOURCE=500 $(CPPFLAGS)

If you would like, I can try removing this line from this GNUmakefile, and the _XOPEN_SOURCE #definitions from json_object.c, json_tokener,c and see how it goes.

I agree it should be a separate issue and PR, but we need to get the present #2395 resolved as well, and the OSX strdup() appears to block it. I might be able to create another branch for the _XOPEN_SOURCE issue, fix it and test locally, and push to you before continuing with the present #2395. Depends on how easily I can subvert the proj shadow problem. I've recently installed Fedora-32 beta, and it's system proj is 6.3.1; I could work from there (later).

[rouault]

OSX seems to be unhappy with rpath: https://travis-ci.com/github/OSGeo/gdal/jobs/319537702 "ld: unknown option: -rpath=-L/usr/local/Cellar/geos/3.8.0/lib"

[eleaver]

Thanks, I saw that geos rpath problem. I'll push a fix, but again please don't merge it yet should it pass.

[rouault]

The failures on Travis-CI are unrelated to your changes, but to a temporary breakage on master. They should be solved by merging latest master into your branch

[eleaver]

I've now removed the -Wl,-rpath[,=]$proj_libdir I had previously added to configure.ac. I did this because I can achieve the same or similar effect in my ConfigureGDAL.sh script by including

proj_libdir=/usr/local/proj-6.3.1/lib
export LDFLAGS+=" -Wl,-rpath=$proj_libdir -L$proj_libdir -lproj "

I do the same for GEOS, but still need to export LD_LIBRARY_PATH+=:/usr/local/geos-3.8.2/lib:
otherwise even with LDFLAGS+=" -Wl,-rpath -Wl,$geos_libdir, my Ubuntu_1804 ld won't find libgeos.so.3.8.2. I might re-visit rpath later, but for now what we've got fixes the original #2395 shadowing issue with minimal code changes, and with your approval should be good to merge.

There remains the _XOPEN_SOURCE=700 issue in ogr/ogrsf_frmts/geojson/libjson/. In GNUmakefile I simply changed the previous 500 value to 700, the wrapped the #define _XOPEN_SOURCE 700 with a #if ! defined(_XOPEN_SOURCE) || _XOPEN_SOURCE < 700 check in json_tokener.c, where it could likely just be removed.

On the one hand, I'd like to revert these changes and see if my builds again fail on OSX, or whether they succeed like everyone else's. And maybe just remove these _XOPEN_SOURCE settings -- both in GNUmakefile and json_tokener.c -- altogether and see if the build succeeds anyway. One might think it should. On the other, someone added that X_OPEN_SOURCE=500 assignment to GNUmakefile for a reason, and if we remove it completely we risk the build breaking for some customer somewhere on some obsolete system that is nonetheless still useful for them, even if the builds succeed on gdal's own Travis CI server.

My current thinking is that if _XOPEN_SOURCE needs to be set at all, it should be done in configure.ac after an AC_CHECK_FUNCS(strdup), similar to the current AC_CHECK_FUNCS(strtof). Under another issue number. By someone who actually has a Mac.

I think this PR is good to merge, but please review and let me know your thoughts. If you approve, we might avoid a bit of noise in the git log if I resubmit just the final result under a new PR from a new local dev branch, you review and merge that, then reject this one.

[rouault]

I've squashed down to a single commit and merged. Thanks

[eleaver]

Thank you Even, and thanks for your patience with this.

[rouault]

@eleaver This seems to cause the issue reported to me by someone building the following Docker image based on quay.io/pypa/manylinux1_x86_64 : https://github.com/DanielFEvans/gdalmanylinux/blob/gdal_3.1.0/Dockerfile.wheels . The symptom is that GDAL linking line completely lacks the -lgeos_c

The relevant excerpt of the build log is:

checking for geos-config... /usr/local/bin/geos-config
checking for GEOS version >= 3.1.0... yes
checking for GEOSversion in -lgeos_c... yes
configure: Using C API from GEOS 3.8.1
sed: invalid option -- E
Usage: sed [OPTION]... {script-only-if-no-other-script} [input-file]...

  -n, --quiet, --silent
                 suppress automatic printing of pattern space
  -e script, --expression=script
                 add the script to the commands to be executed
  -f script-file, --file=script-file
                 add the contents of script-file to the commands to be executed
  -i[SUFFIX], --in-place[=SUFFIX]
                 edit files in place (makes backup if extension supplied)
  -c, --copy
                 use copy instead of rename when shuffling files in -i mode
		 (avoids change of input file ownership)
  -l N, --line-length=N
                 specify the desired line-wrap length for the `l' command
  --posix
                 disable all GNU extensions.
  -r, --regexp-extended
                 use extended regular expressions in the script.
  -s, --separate
                 consider files as separate rather than as a single continuous
                 long stream.
  -u, --unbuffered
                 load minimal amounts of data from the input files and flush
                 the output buffers more often
      --help     display this help and exit
      --version  output version information and exit

If no -e, --expression, -f, or --file option is given, then the first
non-option argument is taken as the sed script to interpret.  All
remaining arguments are names of input files; if no input files are
specified, then the standard input is read.

E-mail bug reports to: bonzini@gnu.org .
Be sure to include the word ``sed'' somewhere in the ``Subject:'' field.

Even my man page on Ubuntu 16.04 doesn't include -E . So either we can find a portable replacement for the regular expression, or we'll have to back it out

[rcoup]

http://blog.dmitryleskov.com/small-hacks/mysterious-gnu-sed-option-e/

Fast forward a few minutes, and I have the answer: it is simply equivalent to “-r“!
/* Undocumented, for compatibility with BSD sed. */

[rouault]

Fast forward a few minutes, and I have the answer: it is simply equivalent to “-r“!

yep. Indeed a GNU vs BSD difference with recent versions of both understanding both possibilities, but not older ones (apparently quite recent Mac BSD sed still doesn't understand -r, whereas this older manylinux1 doesn't understand -E). Proposed PR fixing this in #2452

[DanielFEvans]

Quick note to say that I've been able to compile RC2 successfully in that docker image - thanks.

(From what I've read, it's intentionally a very old environment to allow maximum cross-distro compatibility when generating Python packages)