backends: Use raw_link_args to check for the need of RPATH

[lantw44]

Function rpaths_for_bundled_shared_libraries assumes it needs RPATH when
linking arguments of an external dependency has exactly one argument and
the only argument is an absolute path to a library file. This was mostly
fine because almost all .pc files use a -L -l pair instead of the full
path of the library, which means pkg-config dependencies almost always
have at least two arguments. However, there are patches landed in the
meson 0.47 cycle which convert -L -l pair returned by pkg-config to the
absolute path of library. If the output of pkg-config includes exactly
one -L argument and one -l argument, it will be converted to exactly one
absolute path by meson and rpaths_for_bundled_shared_libraries will
assume it needs RPATH. Since meson passes both -rpath and -rpath-link to
the linker and -rpath-link has precedence over LD_LIBRARY_PATH, it
changes the search order of dependent libraries in an unexpected way and
it causes a lot of linking troubles in JHBuild environments on FreeBSD.

To make the method behave like the old way of using -L -l pairs and
avoid library path order problems, we use raw_link_args instead of
link_args here. raw_link_args stores the unmodified output of pkg-config
and it is much less likely to accidentally match the rule currently used
by the method.

Works around #4270.

[lantw44]

Ping ... It is a one-line patch which fixes a lot of build problems, at least in JHBuild environments. What can I do to get it reviewed sooner? It seems to me that patches related to library paths usually takes a few weeks before getting the first review.

I know it is too late to provide a regression fix for meson 0.47, and I am often slow on providing patches for issues which take hours, or even days, to debug. But I still hope we can get it merged soon so it is less likely for us to have to carry meson patches in FreeBSD ports and FreeBSD CI machines for the next meson release.

[nirbheek]

I am not sure if this fix is actually correct. If we're linking to a library with an absolute path, how does setting the RPATH to that location break things? What does it break and how?

[lantw44]

I am not sure if this fix is actually correct. If we're linking to a library with an absolute path, how does setting the RPATH to that location break things? What does it break and how?

I described the problem in #4270.

The reason that it becomes a problem is that meson 0.47 and later versions convert -L -l arguments returned by pkg-config to absolute paths, so absolute paths can come from any .pc files. If we have PKG_CONFIG_PATH set to two or more directories, such as /home/mesonuser/.local/lib/pkgconfig:/usr/local/lib/pkgconfig, and there is a .pc file from /usr/local/lib/pkgconfig having its arguments converted to a single absolute path, -rpath-link /usr/local/lib will be added and it will cause the linker to search /usr/local/lib before /home/mesonuser/.local/lib, which is likely to cause undefined reference errors.

[nirbheek]

Hum, so perhaps a better fix is to set the order of the -rpath-link arguments correctly? It should mirror the order of the -L arguments overall?

[lantw44]

Hum, so perhaps a better fix is to set the order of the -rpath-link arguments correctly?

I am not sure what is the most correct or expected way for users to specify library path order in meson. It doesn't seem to be documented so I always assume if autotools can work in an environment, meson should work in it, too. This means in order to get things to build and run in a two-or-more-prefix setup, these environment variables should be set in correct orders:

1. PKG_CONFIG_PATH to set the expected order of searching libraries. There is currently a minor issue of meson here and I proposed a fix in PkgConfigDependency: Sort -L flags according to PKG_CONFIG_PATH #4325.
2. LDFLAGS: to override the order of -L flags returned by pkg-config because pkg-config does not and can not order it in a meaningful way. It has been made to work with Preserve -L -l pairings fetched from external deps #1932, Fix -L order, LDFLAGS, LD_LIBRARY_PATH issues in GNOME module #2800, gnome: Distinguish between internal and external linker flags #3463, and maybe other patches.
3. LD_LIBRARY_PATH: to tell both the build time linker and the run time linker where they can find dependencies if they can not find them in -rpath-link or DT_RPATH.

In my test environment, they are all handled by JHBuild.

Since we already have LD_LIBRARY_PATH, adding more things to -rpath-link is not really useful. But if we can get it set in the correct order, it should not cause troubles as well. I know both g-ir-scanner and GNOME module of meson already do this, and they seem to work fine.

It should mirror the order of the -L arguments overall?

Yes if we specify all libraries with -L and -l arguments like what we did in the GNOME module. No if we have converted -L and -l to absolute paths because we also have to consider what these absolute paths originally represent. Even if we treat absolute paths like -L -l pairs and put -L parts into RPATH, it will still require more work to make it really work.

For example, this is what meson with the current patch applied generates for atk:

build tests/testdocument: c_LINKER tests/tests@@testdocument@exe/testdocument.c.o | /usr/local/lib/libintl.a /usr/local/lib/libintl.so.8.1.5 /home/lantw44/gnome/devinstall/lib/libgobject-2.0.so /home/lantw44/gnome/devinstall/lib/libglib-2.0.so atk/atk@@atk-1.0@sha/libatk-1.0.so.0.29303.1.symbols

LINK_ARGS = -L/home/lantw44/gnome/devinstall/lib -L/usr/local/lib -Wl,--no-undefined -Wl,--as-needed -march=corei7 -B/home/lantw44/.local/bin -pipe -g3 -O0 -gz -fdebug-macro -B/home/lantw44/.local/bin -Wl,--compress-debug-sections=zlib -Wl,--start-group atk/libatk-1.0.so.0.29303.1 -L/home/lantw44/gnome/devinstall/lib /home/lantw44/gnome/devinstall/lib/libgobject-2.0.so /home/lantw44/gnome/devinstall/lib/libglib-2.0.so -lintl -Wl,--end-group '-Wl,-rpath,$$ORIGIN/../atk' -Wl,-rpath-link,/home/lantw44/gnome/build/atk/atk

My LDFLAGS is -L/home/lantw44/gnome/devinstall/lib -L/usr/local/lib -B/home/lantw44/.local/bin -Wl,--compress-debug-sections=zlib.

If we simply follow the order of -L and absolute path arguments here, it will become -Wl,-rpath-link,/home/lantw44/gnome/devinstall/lib:/usr/local/lib:/home/lantw44/gnome/build/atk/atk, which is wrong because it makes external dependencies more important than internal ones. I guess we will have to do things similar to #3463 if we want to add all -L paths to RPATH.

[nirbheek]

Since we already have LD_LIBRARY_PATH, adding more things to -rpath-link is not really useful

This is where a lot of people disagree. The argument is that you should not need to specify LD_LIBRARY_PATH to get your binary to run.

Also on macOS, DYLD_LIBRARY_PATH can no longer be set in new shells as a security measure, so we're left with RPATHs as the only way. I should add a test for this so that the CI doesn't show up green when this behaviour changes.

My LDFLAGS is

Note that those are not read while resolving pkg-config libraries because the pkg-config file is supposed to be the canonical source of information for where to find a library.

which is wrong because it makes external dependencies more important than internal ones

This is easy to fix: just ensure that paths pointing to the builddir are specified first.

[lantw44]

Since we already have LD_LIBRARY_PATH, adding more things to -rpath-link is not really useful

This is where a lot of people disagree. The argument is that you should not need to specify LD_LIBRARY_PATH to get your binary to run.

I don't object to the idea of adding more RPATH as long as it works properly. If we add paths which can be easily handled by LD_LIBRARY_PATH to RPATH, will them be kept on installation? If the answer is yes, it may be considered a behavior change.

Also on macOS, DYLD_LIBRARY_PATH can no longer be set in new shells as a security measure, so we're left with RPATHs as the only way. I should add a test for this so that the CI doesn't show up green when this behaviour changes.

Do you mean the 'binary should run without LD_LIBRARY_PATH' case here?

My LDFLAGS is

Note that those are not read while resolving pkg-config libraries because the pkg-config file is supposed to be the canonical source of information for where to find a library.

Yes, -L flags in LDFLAGS are only used to find -lintl in this case. Meson is unable to convert it to an absolute path because it is not in the -L directory of the .pc file.

which is wrong because it makes external dependencies more important than internal ones

This is easy to fix: just ensure that paths pointing to the builddir are specified first.

Yes, it should be easily resolved by tracking where these flags come from like the GNOME module.

[lantw44]

@nirbheek What should I do in order to move forward on this issue? Add more -rpath so all external libraries have not only -L but also -rpath?

[lantw44]

@jpakkane @nirbheek Ping again. What is the recommended way to resolve the issue?

[lantw44]

@jpakkane @nirbheek Ping! Please tell me what should I do here. Adding unnecessary -rpath like what commit ec45c29 does is very likely to cause troubles for GNU ld. You can find the explanation by running man ld and searching for -rpath-link. Personally I prefer dropping the special case added by commit ec45c29, but the patch I proposed here is the minimum change required to avoid the issue. I think it should be easy for users to add the required -rpath arguments themselves, but it is hard for a buid system to guess if RPATH is needed and add it in the correct order.

Since the pull request is now opened for one year and it is required for JHBuild to work properly, I am going to submit this patch for inclusion in JHBuild. It is annoying to have to remember to apply patches every time Meson is updated.

[lantw44]

@jpakkane @nirbheek Ping again! What should I do in order to get a response?

[lantw44]

Rebased on the master branch. Let's hope someone is willing to accept it or provide a different fix.

[mensinda]

I don't know enough about the backend and even less about the magic of RPATHs to comment on whether this is correct or not. But seeing that five pings were ignored, I would suggest asking on the #mesonbuild IRC to get this either rereviewed, merged or closed.

[dankegel]

Did #7103 address any of these concerns?

[lantw44]

Did #7103 address any of these concerns?

No, #7103 is about adding user-provided RPATH, but this pull request is to stop meson from adding unnecessary RPATH, which is known to cause linking and test failure. It is a regression of meson 0.47.

[codecov]

Codecov Report

Merging #4324 (c766155) into master (f407ad5) will decrease coverage by 3.18%.
The diff coverage is n/a.

❗ Current head c766155 differs from pull request most recent head c59e5d1. Consider uploading reports for the commit c59e5d1 to get more accurate results

@@            Coverage Diff             @@
##           master    #4324      +/-   ##
==========================================
- Coverage   66.73%   63.54%   -3.19%     
==========================================
  Files         378      189     -189     
  Lines       84597    42236   -42361     
  Branches    17501     8738    -8763     
==========================================
- Hits        56455    26840   -29615     
+ Misses      23352    13043   -10309     
+ Partials     4790     2353    -2437     

Impacted Files	Coverage Δ
linkers/linkers.py	54.05% <0.00%> (-0.27%)	⬇️
backend/backends.py	81.84% <0.00%> (-0.18%)	⬇️
compilers/compilers.py	79.93% <0.00%> (-0.16%)	⬇️
mesonbuild/dependencies/base.py
mesonbuild/ast/printer.py
mesonbuild/cmake/toolchain.py
mesonbuild/scripts/externalproject.py
mesonbuild/modules/unstable_cuda.py
mesonbuild/interpreter/__init__.py
mesonbuild/scripts/tags.py
... and 181 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f407ad5...c59e5d1. Read the comment docs.

[lantw44]

Rebased to the latest commit and ping again.