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: Use pkg-config generator and some cleanups #3351

Merged
merged 3 commits into from
Mar 1, 2022

Conversation

xclaesse
Copy link
Contributor

When using library() instead of shared_library() and static_library,
meson will build shared, static, or both depending on the
value of static_library option.

As far as I know extract_all_objects() was uses as workaround for Meson
bugs fixed a while ago when using not installed static libraries.

@xclaesse
Copy link
Contributor Author

@tp-m FYI, this is needed to get openh264 static linked into gstreamer-full.

@nanonyme
Copy link

nanonyme commented Dec 8, 2020

@xclaesse are you sure that is sensible in general? Only Cisco is allowed to distribute OpenH264 binaries with the patent license so static libraries are problematic for most parties.

@xclaesse
Copy link
Contributor Author

xclaesse commented Dec 8, 2020

@nanonyme on the contrary, by default openh264 currently installs both shared a static libraries. With this PR by default it will only install shared library because meson defaults to default_library=shared. You have to build with --default-library=both or --default-library=static to get the static library with my change.

@xclaesse
Copy link
Contributor Author

xclaesse commented Dec 8, 2020

That being said, I didn't check the license, I'm not distributing openh264 I just saw this inconsistency in the build system with my Meson developer hat ;-)

@nanonyme
Copy link

nanonyme commented Dec 8, 2020

Right. I was largely referring to that ffmpeg-full use case you mentioned. Expected way is someone builds openh264, Cisco puts it available and end user downloads it directly from Cisco where it's plugged to ffmpeg or whatever. So from that point of view even allowing static linkage seems strange. Maybe there simply should be no static version? Does anyone need it? It seems like a legal footgun to me.

@thaytan
Copy link
Contributor

thaytan commented Dec 8, 2020

If someone is building OpenH264 for themselves they should already know the H.264 licensing situation. Dynamic or static library build doesn't make a difference - the code license permits either.

If you want to take advantage of Cisco's H.264 licensing you need to get the binary from them. For iOS a static library is appropriate and provided:

https://github.com/cisco/openh264/releases

@nanonyme
Copy link

nanonyme commented Dec 8, 2020

@thaytan are you sure iOS usage is appropriate? It does not sound possible to fulfill the requirement there that customer gets OpenH264 through Cisco (not developer but customer). The patent grant is not generally considered to allow redistribution.

@tp-m
Copy link
Contributor

tp-m commented Dec 8, 2020

@nanonyme I don't think this is an accurate depiction of the legal situation at all to be honest, it only reflects a particular distro perspective/pov really. But even if it were there are many people that do not distribute the software they build to third parties but simply use it locally. Lots of other people can distribute their own openh264 builds entirely legally too, it just happens that Cisco makes builds available for distros to use. Anyway, I think this consideration is entirely irrelevant to this PR anyway to be honest :)

The PR is a clean-up that embodies meson best practices, and if it works correctly it should be merged.

@eli-schwartz
Copy link
Contributor

eli-schwartz commented Aug 9, 2021

Fixed by mesonbuild/meson#5936, specifically.

This requires bumping the minimum version of meson to 0.52 to guarantee that no one tries building it using versions of meson from before the fix.

Other than that... this results in a more idiomatic meson.build with a dozen fewer lines, so it seems like a no-brainer to merge :) the licensing situation seems quite tangential.

@eli-schwartz
Copy link
Contributor

@xclaesse Could you bump the minimum meson version?

@xclaesse
Copy link
Contributor Author

@xclaesse Could you bump the minimum meson version?

done.

@nanonyme I think we answered your concerns, do you think we can go ahead with this PR?

xclaesse added a commit to xclaesse/wrapdb that referenced this pull request Oct 23, 2021
It has an upstream meson build system. Unfortunately it has many meson
warnings, but they are all being fixed upstream for the next release:
cisco/openh264#3351
xclaesse added a commit to xclaesse/wrapdb that referenced this pull request Oct 23, 2021
It has an upstream meson build system. Unfortunately it has many meson
warnings, but they are all being fixed upstream for the next release:
cisco/openh264#3351
xclaesse added a commit to xclaesse/wrapdb that referenced this pull request Oct 23, 2021
It has an upstream meson build system. Unfortunately it has many meson
warnings, but they are all being fixed upstream for the next release:
cisco/openh264#3351
xclaesse added a commit to xclaesse/wrapdb that referenced this pull request Oct 23, 2021
It has an upstream meson build system. Unfortunately it has many meson
warnings, but they are all being fixed upstream for the next release:
cisco/openh264#3351

Disable tests for now because they take a long time and fails.
xclaesse added a commit to xclaesse/wrapdb that referenced this pull request Oct 23, 2021
It has an upstream meson build system. Unfortunately it has many meson
warnings, but they are all being fixed upstream for the next release:
cisco/openh264#3351

Disable tests for now because they take a long time and fails.
@xclaesse
Copy link
Contributor Author

xclaesse commented Nov 8, 2021

@nanonyme kind reminder to review this PR please :)

@nanonyme
Copy link

nanonyme commented Nov 8, 2021

Sure, do go ahead. I wasn't aware I was a blocker.

@xclaesse
Copy link
Contributor Author

xclaesse commented Nov 8, 2021

I don't have merge permission, who does?

@GuangweiWang
Copy link
Collaborator

@xclaesse
CI build failed. can you help to check?

@eli-schwartz
Copy link
Contributor

@GuangweiWang The CI failure is because commit d5dbb59 (and the current state of the github actions workflow) pins an exact version of meson to install in the CI.

This pre-existing PR bumped the minimum version of meson from 0.50 to 0.52 and thus meson failed because it detected that the version installed from pip install is too old to handle the project.

I'm not sure I understand the point of pinning an old version here. If you just want to prevent breaking the minimum version, meson warns you if you use too new features (and you can make that into failure by passing --fatal-meson-warnings).

@GuangweiWang
Copy link
Collaborator

failed reason:
Run meson builddir
The Meson build system
Version: 0.50.1
Source dir: /home/runner/work/openh264/openh264
Build dir: /home/runner/work/openh264/openh264/builddir
Build type: native build

meson.build:1:0: ERROR: Meson version is 0.50.1 but project requires >= 0.52.0.

A full log can be found at /home/runner/work/openh264/openh264/builddir/meson-logs/meson-log.txt
Error: Process completed with exit code 1.

@xclaesse xclaesse force-pushed the meson-library branch 2 times, most recently from 73a0e53 to ba0e3f4 Compare January 31, 2022 14:07
@xclaesse xclaesse changed the title meson: Respect default_library option meson: Use pkg-config generator and some cleanups Jan 31, 2022
@xclaesse
Copy link
Contributor Author

Rebased this PR and removed the parts that got already merged in separate PRs.

GuangweiWang pushed a commit that referenced this pull request Feb 18, 2022
In #3351, some project cleanups were done, which included migrating from
a workaround for a bug that used extract_all_objects, to using
link_whole + modern meson instead.

The bug fix was supposed to fix building libopenh264 as a static
library, since link_whole (and link_with) never correctly linked the
actual objects from another static library.

Sadly this never got merged. Instead #3458 (via commit
ce3f53a) was merged, which changed to
link_with instead. This is not supposed to work, because shared
libraries created by converting a static library (or 3) into a shared
library *need* -Wl,--whole-archive.

(It did work for building libopenh264 as a static library, because part
of the meson bug fix to link actual objects ensured that linking one
static library into another behaves the same whether you use link_with
or link_whole. This is needed since static libraries aren't pre-linked.)

As a result, libopenh264.so was created with zero original objects, and
linked to static libraries that provided many symbols, none of which
were required for original symbols, and thus none of which were actually
included in the final shared library.

Fix by using link_whole as originally intended.

Fixes #3475
@GuangweiWang
Copy link
Collaborator

@xclaesse can you help to resolve the conflicts? thanks

@xclaesse
Copy link
Contributor Author

@xclaesse can you help to resolve the conflicts? thanks

done.

While at it, what's the plan for the next release? Currently the latest release to totally broken when building with Meson unfortunately.

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.

6 participants