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

Fix ninja cannot find the library when libraries contain symlinks. #12808

Merged
merged 2 commits into from
Apr 22, 2024
Merged

Fix ninja cannot find the library when libraries contain symlinks. #12808

merged 2 commits into from
Apr 22, 2024

Conversation

U2FsdGVkX1
Copy link
Contributor

@U2FsdGVkX1 U2FsdGVkX1 commented Feb 4, 2024

If there're weird symlinks in the system like Fedora RISC-V
/lib64/lp64d -> /lib64
and glibc is installed to /lib64/lp64d

When calling cxx.find_library, it will first search for /lib64/lp64d/../lib64/lp64d/

dirs = self.get_compiler_dirs(env, 'libraries')

result.append(unresolved)

Then generate linkargs ['/lib64/lp64d/../lib64/lp64d/libbz2.so']
which will cause ninja build fail: ninja-build/ninja#1330

Here is a simple example

meson.build:
project('my_project', 'c')
c_compiler = meson.get_compiler('c')
bzip2_dep = c_compiler.find_library(
      'bz2',
      dirs: '/usr/lib',
)
executable('main', 'main.c', dependencies: bzip2_dep)

main.c:
int main(){return 0;}
sh-5.2# meson setup build
The Meson build system
Version: 1.3.1
Source dir: /test
Build dir: /test/build
Build type: native build
Project name: my_project
Project version: undefined
C compiler for the host machine: cc (gcc 13.2.1 "cc (GCC) 13.2.1 20231011 (Red Hat 13.2.1-4)")
C linker for the host machine: cc ld.bfd 2.41-14
Host machine cpu family: riscv64
Host machine cpu: riscv64
linkargs ['/lib64/lp64d/../lib64/lp64d/libbz2.so']
Library bz2 found: YES
Build targets in project: 1

Found ninja-1.11.1 at /usr/bin/ninja
sh-5.2# ninja -C build/
ninja: Entering directory `build/'
ninja: error: '/lib64/lib64/lp64d/libbz2.so', needed by 'main', missing and no known rule to make it

According to the commit history, the issue was introduced in this commit that inserts the unresolved variable to paths to avoid systemd test fail.
I don't know if the workaround mentioned above is still required now, so I temporarily adjusted the priority in my patch for compatibility.

CC: @rwmjones @davidlt

@eli-schwartz
Copy link
Member

If the system has weird symlinks like in Fedora RISC-V
/lib64/lp64d -> /lib64
and glibc is installed to /lib64/lp64d

What exactly is the purpose of such a symlink, anyway?

@U2FsdGVkX1
Copy link
Contributor Author

U2FsdGVkX1 commented Feb 4, 2024

If the system has weird symlinks like in Fedora RISC-V
/lib64/lp64d -> /lib64
and glibc is installed to /lib64/lp64d

What exactly is the purpose of such a symlink, anyway?

glibc is installed to /lib64/lp64d or /lib64/ilp32 by default on RISC-V, instead of the common /lib64
so Fedora symlink it to /lib64 for some reasons.

@eli-schwartz
Copy link
Member

That doesn't tell me the purpose of the symlink...

It sounds like a Fedora bug, frankly.

@rwmjones
Copy link

rwmjones commented Feb 4, 2024

The symlink is weird, but the path is perfectly correct and resolvable:

$ realpath /lib64/lp64d/../lib64
/usr/lib64

It's just generated as a side effect of how GCC concatenates its directory paths. Combined with the fact that on RISC-V (any Linux distro) GCC uses subdirectories for each RISC-V subarch, but Linux distros want to put their libraries in /usr/lib or /usr/lib64 (to be FSS compliant) instead.

The same thing could happen on any platform that uses a particular type of symbolic link in GCC search paths, so it's clearly a meson bug that it cannot resolve this type of symlink.

Note also libtool, cmake, etc (in fact every other build system that Linux uses) has no problem here.

@eli-schwartz
Copy link
Member

The same thing could happen on any platform that uses a particular type of symbolic link in GCC search paths, so it's clearly a meson bug that it cannot resolve this type of symlink.

"A particular type of symbolic link" is definitely one way to describe an infinite symlink loop.

Not sure I agree with the "perfectly correct" part though.

@davidlt
Copy link

davidlt commented Feb 4, 2024

This is a valid symlink, readlink and realpath can properly canonicalize the path.

The symlink exist so that we could have both /usr/lib64 and /usr/lib64/lp64d pointing to the same content on RISCV platforms. RISCV is not compliant to File System Hierarchy Standard (FHS). That's because they are using /usr/lib64/<ABI> paths. Historically there are even binutils version that don't even fallback to /usr/lib64 on riscv64. We have this symlink to avoid various issues, but some (not all) meson packages fail to build by producing wrong paths.

@rwmjones
Copy link

rwmjones commented Feb 4, 2024

It's not an infinite symlink loop.

@rwmjones
Copy link

rwmjones commented Feb 4, 2024

Looking at the current (pre-patch) code:

                try:
                    resolved = pathlib.Path(p).resolve().as_posix()
                    if resolved not in result:
                        result.append(resolved)
                except FileNotFoundError:
                    pass

what is it that can raise FileNotFoundError there? It seems as if the pathlib resolve function only raises that if the strict=True flag is passed. as_posix doesn't appear to raise any exceptions.

Since we are expecting all these paths to exist, why not just resolve them all using strict=True, discarding any which throw FileNotFoundError? The current code looks overcomplicated and wrong, but I may be missing something.

@rwmjones
Copy link

rwmjones commented Feb 4, 2024

I confirmed by testing it that the patch posted by @U2FsdGVkX1 does fix the issue on Fedora/RISC-V. (I'm not claiming it's the best or only way to fix the problem though, as I'm still not quite sure I understand what that function is doing.)

@eli-schwartz
Copy link
Member

eli-schwartz commented Feb 4, 2024

It's not an infinite symlink loop.

It was claimed above, that the following is a symlink:

/lib64/lp64d -> /lib64

Looking at the current (pre-patch) code:

                try:
                    resolved = pathlib.Path(p).resolve().as_posix()
                    if resolved not in result:
                        result.append(resolved)
                except FileNotFoundError:
                    pass

what is it that can raise FileNotFoundError there? It seems as if the pathlib resolve function only raises that if the strict=True flag is passed. as_posix doesn't appear to raise any exceptions.

The answer is, as with all good answers, "it depends".

Perhaps if you check the date the code was added it can provide a hint.

@rwmjones
Copy link

rwmjones commented Feb 5, 2024

An infinite loop symlink points to itself, eg:

$ ln -sf link link
$ realpath ./link
realpath: ./link: Too many levels of symbolic links

This symlink does not point to itself, and realpath is able to resolve it without loops.

Perhaps if you check the date the code was added it can provide a hint.

It was added in aa20c91 (2018) and in Python 3.6 the strict default was changed. So that indicates meson should have added strict=True if you wanted to keep the same behaviour.

As Python 3.6 was released in Dec 2016 (over 7 years ago) it could be time to raise the minimum version from 3.5 to 3.6, which would allow the strict=True parameter to be added safely and the exception to be thrown and caught.

@U2FsdGVkX1: The patch needs to be updated to deal with this.

@U2FsdGVkX1
Copy link
Contributor Author

An infinite loop symlink points to itself, eg:

$ ln -sf link link
$ realpath ./link
realpath: ./link: Too many levels of symbolic links

This symlink does not point to itself, and realpath is able to resolve it without loops.

Perhaps if you check the date the code was added it can provide a hint.

It was added in aa20c91 (2018) and in Python 3.6 the strict default was changed. So that indicates meson should have added strict=True if you wanted to keep the same behaviour.

As Python 3.6 was released in Dec 2016 (over 7 years ago) it could be time to raise the minimum version from 3.5 to 3.6, which would allow the strict=True parameter to be added safely and the exception to be thrown and caught.

@U2FsdGVkX1: The patch needs to be updated to deal with this.

I have updated the patch and it also fixed the symlink issue

try:
resolved = pathlib.Path(p).resolve().as_posix()
resolved = pobj.resolve(True).as_posix()
Copy link

Choose a reason for hiding this comment

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

I think this won't work on Python 3.5 as it won't be expecting the strict argument to be present at all. Not sure how to solve that except by bumping the minimum version or introspecting somehow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this won't work on Python 3.5 as it won't be expecting the strict argument to be present at all. Not sure how to solve that except by bumping the minimum version or introspecting somehow.

meson now requires Python 3.7 minimum version

Copy link

Choose a reason for hiding this comment

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

Indeed you are right - I still had an old branch checked out while I was looking for the commit above. So yes, this should be fine.

@jpakkane jpakkane merged commit f233b7b into mesonbuild:master Apr 22, 2024
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants