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

Make: add -rpath to LD_SEARCH_FLAGS #2280

Merged
merged 3 commits into from
Apr 11, 2022
Merged

Make: add -rpath to LD_SEARCH_FLAGS #2280

merged 3 commits into from
Apr 11, 2022

Conversation

metzm
Copy link
Contributor

@metzm metzm commented Mar 25, 2022

Currently, the GDAL-GRASS plugin works only if the directory with GRASS libraries is added to LD_LIBRARY_PATH, otherwise GRASS libraries don't find each other. This PR, together with the corresponding GDAL PR 5503, avoids the need to set LD_LIBRARY_PATH or add an entry to /etc/ld.so.conf.d/, instead the GDAL-GRASS plugin works out of the box without further adjustments to the system's paths with libraries in non-standard locations.

The crucial change is in aclocal.m4. Running autoconf version 2.69 resulted in more changes to configure than expected, I am not sure if the added runstatedir could cause any harm. My tests succeeded so far.

How to test

Failure with:

  1. build and install GRASS without this PR
  2. build and install the GDAL-GRASS plugin
  3. with the NC sample dataset, try ogrinfo -so /path/to/grassdata/nc_spm_08/PERMANENT/vector/boundary_county/head
    The GDAL error is that some shared GRASS libraries are not found.

Success with:

  1. build and install GRASS with this PR
  2. build and install the GDAL-GRASS plugin with the GDAL PR 5503
  3. with the NC sample dataset, try ogrinfo -so /path/to/grassdata/nc_spm_08/PERMANENT/vector/boundary_county/head
    No GDAL errors.

@metzm metzm added the bug Something isn't working label Mar 25, 2022
@metzm metzm added this to the 8.2.0 milestone Mar 25, 2022
@metzm metzm requested a review from nilason March 25, 2022 18:36
@nilason
Copy link
Contributor

nilason commented Mar 25, 2022

@nilason
Copy link
Contributor

nilason commented Mar 25, 2022

Discovered similar “addition” happen with #2225 .

@sebastic
Copy link
Contributor

The runstatedir option was added to autoconf back in 2013:

http://git.savannah.gnu.org/gitweb/?p=autoconf.git;a=commitdiff;h=a197431414088a417b407b9b20583b2e8f7363bd;hp=f5b1ea679360db7a6714bbf3953f4fe1a3174844

GRASS did not support autoconf newer than 2.13 until recently (7.8.6).

@nilason
Copy link
Contributor

nilason commented Mar 25, 2022

The GRASS´ configure is generated by 2.69. runstatedir is added with 2.70. Note, the 2.69 was released in 2012. That's why it has been patched on some distributions.

@nilason
Copy link
Contributor

nilason commented Mar 25, 2022

With unpatched autoconf 2.69, the result is:

--- configure.orig
+++ configure
@@ -4032,7 +4032,7 @@
 	    SHLIB_SUFFIX=".so"
 	    SHLIB_LD="${CC} -shared"
             LDFLAGS="-Wl,--export-dynamic"
-            LD_SEARCH_FLAGS='-Wl,-rpath-link,${LIB_RUNTIME_DIR}'
+            LD_SEARCH_FLAGS='-Wl,-rpath-link,${LIB_RUNTIME_DIR} -Wl,-rpath,${INST_DIR}/lib'
             LD_LIBRARY_PATH_VAR="LD_LIBRARY_PATH"
             ;;
         *-pc-cygwin)

The macOS build has, as you may have noticed, a similar set of flags for rpath based linking, so it looks sound in my eyes. Cannot test though.

@nilason
Copy link
Contributor

nilason commented Mar 26, 2022

As a general note regarding autoconf version. Presently with GRASS, with a readily generated configure, it might be worth considering bumping to 2.71, as this dependency is only restricted for actual updating of the configure file (not building GRASS). I.e., only devs need a autoconf 2.71.

@metzm
Copy link
Contributor Author

metzm commented Mar 27, 2022

The macOS build has, as you may have noticed, a similar set of flags for rpath based linking, so it looks sound in my eyes. Cannot test though.

Yes, but the BSD builds were missing this rpath setting, in case of netbsd it was wrong, now fixed.

Copy link
Contributor

@nilason nilason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good with the changes related to the main issue, but I advice you to remove the runstatedir additions. They will be removed after next regeneration by a non-patched autoconf.

@metzm
Copy link
Contributor Author

metzm commented Mar 30, 2022

I'm good with the changes related to the main issue, but I advice you to remove the runstatedir additions. They will be removed after next regeneration by a non-patched autoconf.

OK, the runstatedir additions have been removed.

@metzm metzm merged commit 03a6bdd into OSGeo:main Apr 11, 2022
@metzm metzm deleted the link_with_rpath branch April 11, 2022 20:45
@neteler
Copy link
Member

neteler commented Apr 12, 2022

@metzm May I suggest to backport this to G8.0.x? Otherwise we'll not see this change for a while, as it takes time for the various distros to pick up the upcoming G8.2.x.

neteler pushed a commit that referenced this pull request Apr 21, 2022
* Make: add -rpath to LD_SEARCH_FLAGS
@neteler neteler modified the milestones: 8.2.0, 8.0.2 Apr 21, 2022
@neteler
Copy link
Member

neteler commented Apr 21, 2022

Backport done to 8.0.x as discussed with @metzm

neteler pushed a commit to neteler/grass that referenced this pull request Apr 21, 2022
* Make: add -rpath to LD_SEARCH_FLAGS
@neteler
Copy link
Member

neteler commented Apr 21, 2022

Backport done to 7.8.x as discussed with @metzm

ninsbl pushed a commit to ninsbl/grass that referenced this pull request Oct 26, 2022
* Make: add -rpath to LD_SEARCH_FLAGS
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Feb 17, 2023
* Make: add -rpath to LD_SEARCH_FLAGS
neteler pushed a commit to nilason/grass that referenced this pull request Nov 7, 2023
* Make: add -rpath to LD_SEARCH_FLAGS
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants