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

Remove LD_LIBRARY_PATH instructions #144

Merged
merged 2 commits into from
Apr 16, 2024
Merged

Remove LD_LIBRARY_PATH instructions #144

merged 2 commits into from
Apr 16, 2024

Conversation

jacobvosmaer
Copy link
Collaborator

@jacobvosmaer jacobvosmaer commented Apr 11, 2024

This simplifies the instructions for developers and our CI settings.

I don't know when this changed but setting LD_LIBRARY_PATH is not
necessary, TAGLIB_DIR works fine and this is simpler.
@robinst
Copy link
Owner

robinst commented Apr 12, 2024

Is it not necessary anymore?

@jacobvosmaer
Copy link
Collaborator Author

@robinst wouldn't CI fail if it was still needed? I also don't need it locally.

@jacobvosmaer
Copy link
Collaborator Author

jacobvosmaer commented Apr 12, 2024

When you run TAGLIB_DIR=/foo/taglib rake compile, the path /foo/taglib is stored in your Ruby C extension. The moment you load the C extension it will use /foo/taglib. Only if you want to use a different taglib location from the one the C extension was compiled with would you need LD_LIBRARY_PATH.

This is why bundle exec rake test works correctly in CI without any information about where taglib is installed: that information is contained in the Ruby C extension files already.

jacobvosmaer@ubuntu:~/taglib-ruby$ ldd tmp/aarch64-linux/taglib_mpeg/3.3.0/taglib_mpeg.so
        linux-vdso.so.1 (0x0000ffffb3858000)
        libruby.so.3.3 => /home/jacobvosmaer/.rubies/3.3.0/lib/libruby.so.3.3 (0x0000ffffb3325000)
        libtag.so.1 => /home/jacobvosmaer/taglib-ruby/tmp/i386-mingw32/taglib-1.11.1//lib/libtag.so.1 (0x0000ffffb321d000)
        libstdc++.so.6 => /lib/aarch64-linux-gnu/libstdc++.so.6 (0x0000ffffb3038000)
        libpthread.so.0 => /lib/aarch64-linux-gnu/libpthread.so.0 (0x0000ffffb3007000)
        libm.so.6 => /lib/aarch64-linux-gnu/libm.so.6 (0x0000ffffb2f5c000)
        libc.so.6 => /lib/aarch64-linux-gnu/libc.so.6 (0x0000ffffb2de9000)
        /lib/ld-linux-aarch64.so.1 (0x0000ffffb3828000)
        libgcc_s.so.1 => /lib/aarch64-linux-gnu/libgcc_s.so.1 (0x0000ffffb2dc5000)
        libz.so.1 => /lib/aarch64-linux-gnu/libz.so.1 (0x0000ffffb2d9b000)
        libdl.so.2 => /lib/aarch64-linux-gnu/libdl.so.2 (0x0000ffffb2d87000)
        libcrypt.so.1 => /lib/aarch64-linux-gnu/libcrypt.so.1 (0x0000ffffb2d3e000)

@jacobvosmaer
Copy link
Collaborator Author

I wonder if the need to use LD_LIBRARY_PATH had to do with the problem solved (in taglib-2, not main) by #131.

Namely, if you have a TagLib installation in /usr/local, then rake compile ignores TAGLIB_DIR even when it is set. So then you would get a Ruby C extension that wants to load TagLib from /usr/local. In that case LD_LIBRARY_PATH could force Ruby to use your TagLib in tmp/ instead. But the solution for that problem is to make rake compile respect TAGLIB_DIR in the first place, not to first compile the wrong path into the C extension and then override it at runtime.

@robinst robinst merged commit c6f788c into main Apr 16, 2024
4 checks passed
@robinst
Copy link
Owner

robinst commented Apr 16, 2024

Was just wondering why we even had it in the first place. Merged!

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.

2 participants