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: Improve library detection #132

Closed
wants to merge 4 commits into from
Closed

meson: Improve library detection #132

wants to merge 4 commits into from

Conversation

madpilot78
Copy link
Contributor

@madpilot78 madpilot78 commented Mar 11, 2023

Fix detection of some libraries in FreeBSD

  • Use pkgconfig to detect faad (using faad2.pc)
  • Look for mp3lame and lirc_client in /usr/local/lib on FreeBSD
  • Remove unneeded link_args: ['-llirc_client'], required arguments provided by cxx.find_library()

@madpilot78 madpilot78 marked this pull request as draft March 11, 2023 18:33
@madpilot78 madpilot78 marked this pull request as ready for review March 11, 2023 18:37
@radioactiveman
Copy link
Member

radioactiveman commented Mar 12, 2023

  1. This has nothing to do with libintl, you only copy-pasted the description from your other pull request.
  2. Is the correct name lirc or lirc_client?
  3. Could we use pkgconfig (using dependency() instead of cxx.find_library() on FreeBSD? I don't like workarounds like these and would prefer a general solution. If needed with a fallback to the current approach to support older distributions.
  4. The filewriter plugin also uses cxx.find_library() to search lame. Does this work on FreeBSD?

@madpilot78
Copy link
Contributor Author

@radioactiveman

  1. This has nothing to do with libintl, you only copy-pasted the description from your other pull request.

Ops sorry, I prepared the commit messages in a separate editor, and must have pasted the wrong text there. I'll fix it later while rebasing this patch.

2. Is the correct name `lirc` or `lirc_client`?

In FreeBSD the library is installed as /usr/local/lib/liblirc_client.so by default. so I guess correct one is lirc_client. I think this is due to the library providing only client utilities.

3. Could we use `pkgconfig` (using `dependency()` instead of `cxx.find_library()` on FreeBSD? I don't like workarounds like these and would prefer a general solution. If needed with a fallback to the current approach to support older distributions.

In this case I used cxx.find_library() since that was what was being used, I can change my patch easily.

As far as I can see lirc_client does not provide a .pc file, so it cannot be detected via pkgconfig.

OTOH faad does provide a .pc file, so I can use dependency() for that.

4. The `filewriter` plugin also uses `cxx.find_library()` to search `lame`. Does this work on FreeBSD?

I admit I did not check this, and it is actually failing to find it, so I will also fix this one.

This library is also not providing a .pc file, so I will need to keep using cxx.find_library().

I'll rebase this patch adding the required fixes.

Thanks for you feedback!

@madpilot78 madpilot78 marked this pull request as draft March 12, 2023 18:15
@madpilot78 madpilot78 changed the title Fix missing libintl Fix some libraries detection in FreeBSD Mar 12, 2023
@madpilot78
Copy link
Contributor Author

Regarding faad, I guess the patch can be generalized and the same detection used for all OSes. I'm unable to test this properly though.

@madpilot78 madpilot78 marked this pull request as ready for review March 12, 2023 21:54
@radioactiveman
Copy link
Member

I have checked the packages on Arch Linux. All of them (faad2, LIRC, lame) ship a pkgconfig file.

My idea was to use an approach like shown below. First search with pkgconfig, then with find_library() as fallback. The dirs parameter can be passed generally. The default dir (like /usr/lib) is then still searched. And the header check can also be included here since we require Meson 0.51 meanwhile.

See also: https://mesonbuild.com/Reference-manual_returned_compiler.html#compilerfind_library

Would you do this for all three libraries? Also include a "meson: " prefix in the commit message. Thanks.
Please test on FreeBSD, our CI does the rest. 😃

diff --git a/src/lirc/meson.build b/src/lirc/meson.build
index 9d214bb31..475f8b00d 100644
--- a/src/lirc/meson.build
+++ b/src/lirc/meson.build
@@ -1,13 +1,17 @@
-lirc_dep = cxx.find_library('lirc', required: false)
-have_lirc = lirc_dep.found() and cxx.has_header('lirc/lirc_client.h')
+lirc_dep = dependency('lirc', required: false)
 
+if not lirc_dep.found()
+  lirc_dep = cxx.find_library('lirc_client', has_headers: 'lirc/lirc_client.h',
+                              dirs: ['/usr/local/lib'], required: false)
+endif
+
+have_lirc = lirc_dep.found()
 
 if have_lirc
   shared_module('lirc',
     'lirc.cc',
     dependencies: [audacious_dep, glib_dep, lirc_dep],
     name_prefix: '',
-    link_args: ['-llirc_client'],
     install: true,
     install_dir: general_plugin_dir
   )

@madpilot78
Copy link
Contributor Author

@radioactiveman Good idea!

As far as I can see the lirc project distribution itself does not include a pc file, and the FreeBSD port is not adding one.

Anyway, sure I'll update the patch with the suggested behavior.

@madpilot78 madpilot78 changed the title Fix some libraries detection in FreeBSD meson: Fix some libraries detection in FreeBSD Mar 12, 2023
@madpilot78 madpilot78 changed the title meson: Fix some libraries detection in FreeBSD meson: Fix libraries detection in FreeBSD Mar 12, 2023
- Try to detect via pkgconfig using `depandency()`
- Fallback to `cxx.find_library()` for library detection adding the `/usr/local/bin` path required by FreeBSD
- Remove unneeded `link_args: ['-llirc_client']`, required arguments provided by detection via `dependency()` or `cxx.find_library()`
@madpilot78 madpilot78 changed the title meson: Fix libraries detection in FreeBSD meson: Improve library detection Mar 12, 2023
@radioactiveman
Copy link
Member

  1. The pkgconfig name is lame, not mp3lame.
  2. Nit-pick but I would add an empty line between the endif and have_[aac,lirc]. 😉
  3. Meson supports LIBRARY_PATH. Does this work for you on FreeBSD?
    Removing the dirs: ['/usr/local/lib'] parameters again would fix the failing Windows CI build.
export LIBRARY_PATH=/usr/local/lib
meson setup build
meson compile -C build

@jlindgren90
Copy link
Member

jlindgren90 commented Mar 13, 2023

I would much prefer the LIBRARY_PATH approach if that works. Hard-coding /usr/local in a bunch of places seems wrong.

@madpilot78
Copy link
Contributor Author

madpilot78 commented Mar 13, 2023

  1. The pkgconfig name is lame, not mp3lame.

I was not aware of this. I'll change that.

2. Nit-pick but I would add an empty line between the `endif` and `have_[aac,lirc]`. wink

3. Meson supports `LIBRARY_PATH`. Does this work for you on FreeBSD?
   Removing the `dirs: ['/usr/local/lib']` parameters again would fix the failing Windows CI build.
export LIBRARY_PATH=/usr/local/lib
meson setup build
meson compile -C build

Should work in FreeBSD, I need to do some testing though, but it does look cleaner on my side too, since, in the FreeBSD ports tree, /usr/local/can be changed by the user a t build time and I have been using substitutions for now.

I'll also update audacious-media-player/audacious#60 ro use LIBRARY_PATH if it works.

@madpilot78
Copy link
Contributor Author

@radioactiveman @jlindgren90

I confirm using LIBRARY_PATH works fine for me, so I have updated the patch.

@jlindgren90
Copy link
Member

Assuming CI passes, this looks good to me.

@radioactiveman
Copy link
Member

Pushed as bf40a4b with minor clean-ups. Thanks again @madpilot78 for your contribution.

@madpilot78 madpilot78 deleted the FreeBSD_meson_build branch March 13, 2023 20:47
freebsd-git pushed a commit to freebsd/freebsd-ports that referenced this pull request Mar 13, 2023
- Expose OPUS option and enable it by default
- Fix lame detection which enables mp3 filewriter plugin (suggested
  by upstream developer Thomas Lange)

Obtained from:	audacious-media-player/audacious-plugins#132
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants