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

meson 0.60 does not set SONAME for shared_module() anymore #9492

Closed
Apteryks opened this issue Oct 29, 2021 · 21 comments · Fixed by #9629
Closed

meson 0.60 does not set SONAME for shared_module() anymore #9492

Apteryks opened this issue Oct 29, 2021 · 21 comments · Fixed by #9629
Milestone

Comments

@Apteryks
Copy link

Apteryks commented Oct 29, 2021

Describe the bug
After upgrading meson from 0.59.1 to 0.60.0, the network-manager build was failing on GNU Guix like so:

starting phase `validate-runpath'
validating RUNPATH of 9 binaries in "/gnu/store/mq8ggp0wybipvnxpwhqw71rgw1k3pkll-network-manager-1.32.12/lib"...
/gnu/store/mq8ggp0wybipvnxpwhqw71rgw1k3pkll-network-manager-1.32.12/lib/NetworkManager/1.32.12/libnm-device-plugin-bluetooth.so: error: depends on 'src/core/devices/wwan/libnm-wwan.so', which cannot be found in RUNPATH ("/gnu/store/2fk1gz2s7ppdicynscra9b19byrrr866-glibc-2.33/lib" "/gnu/store/6p74cpb86qp6h559ck96593bn2x6iq76-glib-2.70.0/lib")
/gnu/store/mq8ggp0wybipvnxpwhqw71rgw1k3pkll-network-manager-1.32.12/lib/NetworkManager/1.32.12/libnm-device-plugin-wwan.so: error: depends on 'src/core/devices/wwan/libnm-wwan.so', which cannot be found in RUNPATH ("/gnu/store/2fk1gz2s7ppdicynscra9b19byrrr866-glibc-2.33/lib" "/gnu/store/6p74cpb86qp6h559ck96593bn2x6iq76-glib-2.70.0/lib")
validating RUNPATH of 5 binaries in "/gnu/store/mq8ggp0wybipvnxpwhqw71rgw1k3pkll-network-manager-1.32.12/libexec"...
validating RUNPATH of 3 binaries in "/gnu/store/mq8ggp0wybipvnxpwhqw71rgw1k3pkll-network-manager-1.32.12/bin"...
validating RUNPATH of 1 binaries in "/gnu/store/mq8ggp0wybipvnxpwhqw71rgw1k3pkll-network-manager-1.32.12/sbin"...
error: in phase 'validate-runpath': uncaught exception:
misc-error #f "RUNPATH validation failed" () #f 
phase `validate-runpath' failed after 0.0 seconds
Backtrace:
           8 (primitive-load "/gnu/store/21szgh3rd49a9dzldpd7wr5v5w9…")
In guix/build/gnu-build-system.scm:
    904:2  7 (gnu-build #:source _ #:outputs _ #:inputs _ #:phases . #)
In ice-9/boot-9.scm:
  1752:10  6 (with-exception-handler _ _ #:unwind? _ # _)
In srfi/srfi-1.scm:
    634:9  5 (for-each #<procedure 7fffee88ee00 at guix/build/gnu-b…> …)
In ice-9/boot-9.scm:
  1752:10  4 (with-exception-handler _ _ #:unwind? _ # _)
In guix/build/gnu-build-system.scm:
   925:23  3 (_)
   568:10  2 (validate-runpath #:validate-runpath? _ # _ #:outputs _)
In ice-9/boot-9.scm:
  1685:16  1 (raise-exception _ #:continuable? _)
  1685:16  0 (raise-exception _ #:continuable? _)

ice-9/boot-9.scm:1685:16: In procedure raise-exception:
RUNPATH validation failed

Reverting meson to 0.59.1 fixes the rpaths.

To Reproduce
Specify custom rpath locations via -Dc_link_args=-Wl,-rpath=/some/path and inspect the resulting binary.

Expected behavior
network-manager should build as it used to with 0.59.1, with correct specified rpaths in the binaries.

system parameters

  • Is this a cross build or just a plain native build (for the same computer)? native build
  • what operating system: Guix System
  • what Python version are you using: 3.9.6
  • what meson --version 0.60
  • what ninja --version if it's a Ninja build 1.10.2
@dcbaker dcbaker added this to the 0.60.1 milestone Oct 29, 2021
@nirbheek
Copy link
Member

I can't reproduce this. With 0.59.4:

$ cd "meson.git/test cases/common/4 shared"
$ rm -rf _build && ../../../0.59/meson.py _build -Dc_link_args=-Wl,-rpath=/some/path && ninja -C _build && readelf -d _build/libmylib.so | grep RUNPATH
The Meson build system
Version: 0.59.4
Source dir: /home/nirbheek/projects/meson/meson.git/test cases/common/4 shared
Build dir: /home/nirbheek/projects/meson/meson.git/test cases/common/4 shared/_build
Build type: native build
Project name: shared library test
Project version: undefined
C compiler for the host machine: ccache cc (gcc 11.2.1 "cc (GCC) 11.2.1 20210728 (Red Hat 11.2.1-1)")
C linker for the host machine: cc ld.bfd 2.35.2-6
Host machine cpu family: x86_64
Host machine cpu: x86_64
Build targets in project: 2

Found ninja-1.10.2 at /usr/bin/ninja
ninja: Entering directory `_build'                                                                  
[4/4] Linking target libmylib.so
 0x000000000000001d (RUNPATH)            Library runpath: [/some/path]

And with Meson 0.60:

$ rm -rf _build && ../../../0.60/meson.py _build -Dc_link_args=-Wl,-rpath=/some/path && ninja -C _build && readelf -d _build/libmylib.so | grep RUNPATH
The Meson build system
Version: 0.60.0
Source dir: /home/nirbheek/projects/meson/meson.git/test cases/common/4 shared
Build dir: /home/nirbheek/projects/meson/meson.git/test cases/common/4 shared/_build
Build type: native build
Project name: shared library test
Project version: undefined
C compiler for the host machine: ccache cc (gcc 11.2.1 "cc (GCC) 11.2.1 20210728 (Red Hat 11.2.1-1)")
C linker for the host machine: cc ld.bfd 2.35.2-6
Host machine cpu family: x86_64
Host machine cpu: x86_64
Build targets in project: 2

shared library test undefined

  User defined options
    c_link_args: -Wl,-rpath=/some/path

Found ninja-1.10.2 at /usr/bin/ninja
ninja: Entering directory `_build'                                                                  
[4/4] Linking target libmylib.so
 0x000000000000001d (RUNPATH)            Library runpath: [/some/path]

@Apteryks
Copy link
Author

Apteryks commented Nov 2, 2021

@nirbheek Hmm, sorry my attempt at a reproducer failed. I hadn't actually tried it precisely that way myself; I simply noticed that the rpath validation phase in Guix would fail when attempting to build NetworkManager with Meson 0.60 and succeed with 0.59.4, as show in the log originally posted. There's probably something else at play then :-(.

@nirbheek nirbheek modified the milestones: 0.60.1, 0.60.2 Nov 2, 2021
@nirbheek
Copy link
Member

nirbheek commented Nov 2, 2021

Have you already reported this to Guix? Or are you a Guix maintainer yourself? In any case, we need more details to be able to do anything here.

@Apteryks
Copy link
Author

Apteryks commented Nov 5, 2021

Hello; I'm one of the army of Guix maintainers, yes :-). I was assuming the problem had to do with Meson since just going back to 0.59.4 makes the issue go away.

Perhaps one thing which we have in Guix which is not going to be found on traditional distributions is that the 'ld' linker binary available on PATH is a wrapper script to the real GNU ld, which is tasked with translating any link directive used during the build into rpath directives (see: https://git.savannah.gnu.org/cgit/guix.git/tree/gnu/packages/ld-wrapper.in?id=2b76179ecd951172288f5f6f78402d9304d2da41#n44). So, if for some reason Meson from 0.59.4 to 0.60 now links with objects in the source directory, that would be captured in the binaries as rpaths because of this script.

Did Meson's behavior change in this regard?

@eli-schwartz
Copy link
Member

To be clear, every version of meson adds rpaths for the build directory (not the source directory). These rpaths are generically speaking useful to ensure that shared libraries which an executable links to, are functional when running the uninstalled version of the program.

There is a depfixer script which runs against installed binaries and is supposed to remove those rpath entries (but leave alone the ones you added as link args). As I don't understand the format of the error message which Guix is displaying... what is the problematic rpath field of the binary that is failing your validation?

@civodul
Copy link

civodul commented Nov 18, 2021

Hello!

@eli-schwartz: The relevant bit is this line:

lib/NetworkManager/1.32.12/libnm-device-plugin-bluetooth.so: error: depends on 'src/core/devices/wwan/libnm-wwan.so', which cannot be found in RUNPATH ("/gnu/store/2fk1gz2s7ppdicynscra9b19byrrr866-glibc-2.33/lib" "/gnu/store/6p74cpb86qp6h559ck96593bn2x6iq76-glib-2.70.0/lib")

What it's saying is that the NEEDED field of libnm-device-plugin-bluetooth.so contains literally src/core/devices/wwan/libnm-wwan.so (rather than libnm-wwan.so). This happens because libnm-wwan.so doesn't have a SONAME and the link command lists src/core/devices/wwan/libnm-wwan.so as is (as opposed to -L... -lnm-wwan).

Indeed, I can see 0.59.4 passes -Wl,-soname=... when building those libraries whereas 0.60.0 does no such thing.

Looking at ninjabackend.py, the difference between these two versions is this extra condition, which comes from d553506 (hi @bonzini! :-)).

I'm not sure how to address this but the bogus NEEDED fields look problematic to me. Thoughts?

@eli-schwartz
Copy link
Member

Huh... Isn't shared_module only supposed to be used for things like dlopened plugins, that are never linked to? Why is networkmanager creating one, then linking to it?

@bonzini
Copy link
Contributor

bonzini commented Nov 18, 2021

Isn't shared_module only supposed to be used for things like dlopened plugins, that are never linked to?

I suppose that libnm-wwan.so needs to link to the executable, but is also a dependency of libnm-bluetooth.so. I think that should be a shared_library as you say, and the bug is in Network manager, but perhaps we need to make the behavior depend on the minimum required meson version?

Can you Guix folks check if changing wwan from module to library fixes it?

@civodul
Copy link

civodul commented Nov 18, 2021

@bonzini Changing shared_module to shared_library doesn't work because then they get linked with -Wl,--no-undefined, leading to link failures.

Note that the problem is not limited to NetworkManager; it's fairly common in the GNOME and Freedesktop stack, which so far we worked around by switching to 0.59: https://git.savannah.gnu.org/cgit/guix.git/log?id=9f94eb3d7bb9dab83d5df80b4f05558abfcc5564

@nirbheek
Copy link
Member

@bonzini Changing shared_module to shared_library doesn't work because then they get linked with -Wl,--no-undefined, leading to link failures.

You should do this:

shared_library('nm-wwan', ...,
    override_options: ['b_lundef=false'])

Can you try that and see if it works? It really should. This feature (override_options:) has been available since meson 0.40.

perhaps we need to make the behavior depend on the minimum required meson version?

Yeah, I think this makes sense.

@nirbheek nirbheek changed the title meson 0.60 breaks usage of -Dc_link_args=-Wl,-rpath=[...] meson 0.60 does not set SONAME for shared_module() anymore Nov 18, 2021
@civodul
Copy link

civodul commented Nov 21, 2021

Hi @nirbheek. I confirm that using shared_library and override_options as you propose solves the problem (I'm not a NetworkManager developer, but I changed the Guix package along these lines.)

For the record, NM specifies this:

  meson_version: '>= 0.47.2',

So I suppose it'd be possible to restore the pre-0.60 behavior in this case, as @bonzini suggested, though it may be surprising that changing the minimum requirement changes the way shared libraries/modules are linked.

mbakke pushed a commit to guix-mirror/guix that referenced this issue Nov 21, 2021
This is another way to address
<mesonbuild/meson#9492> as suggested by
Nirbheek Chauhan and Paolo Bonzini.

* gnu/packages/patches/network-manager-meson.patch: New file.
* gnu/local.mk (dist_patch_DATA): Add it.
* gnu/packages/gnome.scm (network-manager)[source]: Use it.
[arguments]: Remove #:meson.
@eli-schwartz
Copy link
Member

eli-schwartz commented Nov 21, 2021

The mesonic approach to this problem, if the sonames linkage change is classified as a breaking change rather than a bug that was fixed, is to add a new kwarg soname: true defaulting to the old behavior, and raise a FeatureDeprecated warning if people don't opt in to the new behavior.

This would mean annoying every single user of shared_module anywhere by asking them to switch to shared_library() or confirm they want a real module (soname: false) but existing code would continue to behave the way it always did. Updating the minimum requirements would result in warnings rather than mysterious behavior changes.

@bonzini
Copy link
Contributor

bonzini commented Nov 21, 2021

Is there even a need to deprecate it? Could the default instead vary depending on the minimum version of Meson that the project requires?

  • version < 0.60: default to soname: true, FeatureNew warning if soname specified
  • version = 0.60(.x): default to soname: false, FeatureNew warning if soname specified
  • version >= 0.61: default to soname: false

@bonzini
Copy link
Contributor

bonzini commented Nov 21, 2021

Or even add a soname automatically if a shared_module is used as a dependency, but (independent of the minimum required version of Meson) raise a FeatureDeprecated warning that shared_library+override_options is preferred.

@eli-schwartz
Copy link
Member

eli-schwartz commented Nov 22, 2021

Detecting the specific case where it's used as a dependency for another target and

  • warning to use shared_library()
  • continuing to add a backwards-compat soname

would be a pretty reasonable option too IMO. The advantage is that we automatically do "the right thing", and get the correct results in each case, and do so no matter what your meson_version is (so people who upgrade their meson copy get better software immediately without having to wait for upstream to drop support for versions of meson that are more than a few days old).

Above, I merely commented on what I think would make more sense in the event we actually wanted to make it depend on your version of meson -- i.e. not a straight behavior change based on meson_version, but a deprecation.

@nirbheek
Copy link
Member

So it looks like we have a way forward now. Detect when shared_module() is used as a library; as link_with: in another target or in declare_dependency(), and in that case:

  1. Emit deprecation warning that shared_library() should be used instead, and that this will be an error in the future
  2. Explain that override_options: ['b_lundef=false'] should be used if the library contains undefined symbols
  3. Add the soname automatically

Just needs someone to implement it now 😉

PS: This issue is the only blocker issue for 0.60.2 without a proposed pull request

@bonzini
Copy link
Contributor

bonzini commented Nov 23, 2021

@civodul, can you confirm that the NetworkManager build is already emitting this warning:

                    mlog.warning('target links against shared modules. This '
                                 'is not recommended as it is not supported on some '
                                 'platforms')

(It should according to https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/28 but it's an old issue)? That would simplify the implementation quite a bit. I don't have much time this week (sorry) but I placed a sketch here.

@bonzini
Copy link
Contributor

bonzini commented Nov 23, 2021

You can already test the Meson patch too - what's missing is mostly running the test suite and/or writing a new test.

@nirbheek
Copy link
Member

@civodul, can you confirm that the NetworkManager build is already emitting this warning:

Yep, I can confirm that that warning is being emitted.

nirbheek added a commit to nirbheek/meson that referenced this issue Nov 24, 2021
Emit a detailed deprecation warning that explains what to do instead.
Also add a unittest.

```
DEPRECATION: target main1 links against shared module linked_module1, which is incorrect.
             This will be an error in the future, so please use shared_library() for linked_module1 instead.
             If the target requires undefined symbols, you can use `override_options: ['b_lundef=false']`.
```

Fixes mesonbuild#9492
@nirbheek
Copy link
Member

I've tested that #9629 fixes the SONAME for libnm-wwan.so in NetworkManager. Someone should really fix NM's build files, it's full of ancient cruft. Lots of warnings, deprecations, and incorrect usage of options 😬

nirbheek added a commit to nirbheek/meson that referenced this issue Nov 24, 2021
Emit a detailed deprecation warning that explains what to do instead.
Also add a unittest.

```
DEPRECATION: target main1 links against shared module linked_module1, which is incorrect.
             This will be an error in the future, so please use shared_library() for linked_module1 instead.
             If the target requires undefined symbols, you can use `override_options: ['b_lundef=false']`.
```

Fixes mesonbuild#9492
nirbheek added a commit to nirbheek/meson that referenced this issue Nov 24, 2021
Emit a detailed deprecation warning that explains what to do instead.
Also add a unittest.

```
DEPRECATION: target prog links against shared module mymod, which is incorrect.
             This will be an error in the future, so please use shared_library() for mymod instead.
             If shared_module() was used for mymod because it has references to undefined symbols,
             use shared_libary() with `override_options: ['b_lundef=false']` instead.
```

Fixes mesonbuild#9492
nirbheek added a commit to nirbheek/meson that referenced this issue Nov 24, 2021
Emit a detailed deprecation warning that explains what to do instead.
Also add a unittest.

```
DEPRECATION: target prog links against shared module mymod, which is incorrect.
             This will be an error in the future, so please use shared_library() for mymod instead.
             If shared_module() was used for mymod because it has references to undefined symbols,
             use shared_libary() with `override_options: ['b_lundef=false']` instead.
```

Fixes mesonbuild#9492
nirbheek added a commit that referenced this issue Nov 24, 2021
Emit a detailed deprecation warning that explains what to do instead.
Also add a unittest.

```
DEPRECATION: target prog links against shared module mymod, which is incorrect.
             This will be an error in the future, so please use shared_library() for mymod instead.
             If shared_module() was used for mymod because it has references to undefined symbols,
             use shared_libary() with `override_options: ['b_lundef=false']` instead.
```

Fixes #9492
nirbheek added a commit that referenced this issue Nov 25, 2021
Emit a detailed deprecation warning that explains what to do instead.
Also add a unittest.

```
DEPRECATION: target prog links against shared module mymod, which is incorrect.
             This will be an error in the future, so please use shared_library() for mymod instead.
             If shared_module() was used for mymod because it has references to undefined symbols,
             use shared_libary() with `override_options: ['b_lundef=false']` instead.
```

Fixes #9492
@nirbheek
Copy link
Member

Apparently linking shared modules to other shared modules is an essential feature on Android: #9999

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants