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

gstreamer: use monorepo build #125996

Merged
merged 18 commits into from
Apr 14, 2023
Merged

gstreamer: use monorepo build #125996

merged 18 commits into from
Apr 14, 2023

Conversation

carlocab
Copy link
Member

@carlocab carlocab commented Mar 17, 2023

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

Upstream have merged all their projects into a monorepo. [1] The
supported way of building the various projects is through the monorepo,
so we should do that.

See Homebrew/discussions#3740 for additional context.

Resolves Homebrew/discussions#3740.

[1] https://gstreamer.freedesktop.org/documentation/frequently-asked-questions/mono-repository.html?gi-language=c

@BrewTestBot BrewTestBot added the automerge-skip `brew pr-automerge` will skip this pull request label Mar 17, 2023
@chenrui333

This comment was marked as resolved.

@carlocab carlocab force-pushed the gstreamer branch 3 times, most recently from 8dff2cb to e85e2e3 Compare March 18, 2023 07:18
@carlocab
Copy link
Member Author

OSError: [Errno 86] Bad CPU type in executable: '/private/tmp/gstreamer-20230318-31006-a2njdq/gstreamer-1.22.1/subprojects/macos-bison-binary/bison-3.7.6-macos-x86_64/bin/bison'

Why are you downloading your own bison? How annoying.

@carlocab carlocab added the CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. label Mar 18, 2023
@carlocab
Copy link
Member Author

carlocab commented Mar 18, 2023

  error[E0069]: `return;` in a function whose return type is not `()`
     --> video/gtk4/src/sink/imp.rs:856:21
      |
  856 |                     return;
      |                     ^^^^^^ return type is not `()`
  
  For more information about this error, try `rustc --explain E0069`.

Not sure why this didn't fail when building gst-plugins-rs previously, though. Different tag.

@carlocab carlocab added the CI-linux-self-hosted Build on Linux self-hosted runner label Mar 18, 2023
Formula/gstreamer.rb Outdated Show resolved Hide resolved
@carlocab carlocab force-pushed the gstreamer branch 6 times, most recently from 64109ff to e77c4da Compare March 18, 2023 20:07
@carlocab carlocab marked this pull request as ready for review March 18, 2023 20:08
Copy link
Member Author

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Ah, wait. This will need a whole lot of link_overwrites, probably.

@carlocab carlocab added the long build Set a long timeout for formula testing label Mar 19, 2023
@SMillerDev SMillerDev added the CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. label Mar 19, 2023
Formula/gstreamer.rb Outdated Show resolved Hide resolved
@carlocab carlocab removed the CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. label Mar 19, 2023
@carlocab carlocab force-pushed the gstreamer branch 2 times, most recently from 541a848 to 084174a Compare March 20, 2023 02:33
@carlocab carlocab added the CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. label Mar 20, 2023
Formula/aravis.rb Outdated Show resolved Hide resolved
@carlocab carlocab added the CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. label Apr 13, 2023
@carlocab carlocab added ready to merge PR can be merged once CI is green and removed CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. labels Apr 13, 2023
@carlocab carlocab requested a review from a team April 14, 2023 01:40
@github-actions
Copy link
Contributor

@github-actions github-actions bot added the CI-published-bottle-commits The commits for the built bottles have been pushed to the PR branch. label Apr 14, 2023
@carlocab carlocab added this pull request to the merge queue Apr 14, 2023
Merged via the queue into Homebrew:master with commit bc184bb Apr 14, 2023
@carlocab carlocab deleted the gstreamer branch April 14, 2023 03:42
Copy link

@nirbheek nirbheek left a comment

Choose a reason for hiding this comment

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

This is great stuff!

system "ninja", "install", "-v"
end
# The apple media plug-in uses API that was added in Mojave
args << "-Dgst-plugins-bad:applemedia=disabled" if MacOS.version <= :high_sierra

Choose a reason for hiding this comment

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

This is not correct, the applemedia plugin supports 10.11 (El Capitan) and newer. That is how the official gstreamer binaries are built. What problems were you having?

Copy link
Member Author

Choose a reason for hiding this comment

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

This line originates from 03d561a, with associated PR at #52730. We don't test on systems older than Big Sur anymore, so I guess this has been fixed since then.

args = %W[
-Dpython=enabled
-Dlibav=enabled
-Dlibnice=disabled

Choose a reason for hiding this comment

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

libnice is actually a really important gstreamer plugin, since it's essential for webrtc support.

I am not sure how you can solve the circular dep though. Maybe you could move libnice into the monorepo too? Is there anything else in Homebrew that uses libnice?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe you could move libnice into the monorepo too?

Yea, maybe. It'll make updating libnice really annoying, though. It's also not clear to me that you get the same libnice if you build it before Gstreamer rather than after, which might be an issue for users.

Is there anything else in Homebrew that uses libnice?

Not currently, no.

Choose a reason for hiding this comment

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

The monorepo will pull in libnice in the same way that it pulls in gst-plugins-rs (both are separate repos that are meson wraps), so it will be identical to building it separately. If nothing else is using libnice, then I suggest merging it into the monorepo too.

Note that libnice is maintained by Olivier, who is a gstreamer core maintainer, and I don't expect anyone outside of gstreamer to use the project.

If you really want to keep it separate, I'd recommend building it twice: once as its own project without gstreamer support, then again as part of the monorepo to build only the gstreamer plugin. That'll require non-trivial build system patching, but it can be done.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not quite the same as gst-plugins-rs, though, which kind of supports updating simultaneously with gstreamer.

If we build libnice with gstreamer, then we need to rebuild all of gstreamer every time we want to update only libnice (if we even remember to update it after it becomes part of gstreamer).

Choose a reason for hiding this comment

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

The libnice wrap file is continuously updated, so you'd get updates whenever you update gstreamer (stable releases for instance). We test the two as a unit in the monorepo and in cerbero.

Copy link

@cooltea713705 cooltea713705 Apr 27, 2023

Choose a reason for hiding this comment

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

Hi! Thanks for documenting the issue: upgrading to 1.22.2 effectively made webrtcbin no longer be listed as plugin. I understand the circular dependency issue, can some work be done to include libnice so that webrtcbin is supported? It's not clear what the best approach is if libnice still needs to be a standalone.

Copy link
Member Author

Choose a reason for hiding this comment

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

Did webrtc work for you before? @nirbheek tells me that it was basically broken previously.

Copy link

@cooltea713705 cooltea713705 Apr 27, 2023

Choose a reason for hiding this comment

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

Did webrtc work for you before?

Up until 1.20.0 at least what it took was for gstreamer + gst-plugins-bad to be installed via brew in which case it was working yes.

Choose a reason for hiding this comment

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

Earlier, all plugin registration was broken unless you set the right env vars, and linking was also broken. webrtcbin was not specifically broken.

I can help you with a solution for the circular dep, just let me know if you need my help.

One way would be to allow gstnice (inside libnice) to be built against a system libnice. The plugin only uses the version string at build time, nothing else.

I can patch that and talk to Olivier about upstreaming it.

Copy link
Member Author

@carlocab carlocab Apr 28, 2023

Choose a reason for hiding this comment

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

One way would be to allow gstnice (inside libnice) to be built against a system libnice. The plugin only uses the version string at build time, nothing else.

That seems like it could work.

If you could point at the right places to patch I'll have a look at applying it to the formula.


bin.env_script_all_files libexec/"bin", GST_PLUGIN_SYSTEM_PATH: HOMEBREW_PREFIX/"lib/gstreamer-1.0"
# Prevent the build from downloading an x86-64 version of bison.
inreplace "meson.build", "subproject('macos-bison-binary')", ""

Choose a reason for hiding this comment

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

Ah, hmm, we should provide a way to disable this. I'll fix it upstream :)

Choose a reason for hiding this comment

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

https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/4428

You can start using this with the 1.24 release.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks!

@spiegela
Copy link

spiegela commented Apr 14, 2023

Interesting, when installing this version, gstreamer pulls in both gtk+3 and gtk+4 for me, but then gst-inspect-1.0 complains about them both being installed:

objc[52356]: Class ResultReceiver is implemented in both /opt/homebrew/Cellar/gtk+3/3.24.37/lib/libgtk-3.0.dylib (0x106cd5df8) and /opt/homebrew/Cellar/gtk4/4.10.1/lib/libgtk-4.1.dylib (0x1159c2ec0). One of the two will be used. Which one is undefined.

edit

nevermind... uninstalling gtk+3 and gtk+4, then re-installing gstreamer seems to have resolved the errors

intel-media-ci pushed a commit to intel-media-ci/gstreamer that referenced this pull request Apr 28, 2023
Similar to orc, allow distros to force bison, flex, nasm, etc, to be
provided by the system.

Needed by the Homebrew folks:
Homebrew/homebrew-core#125996 (comment)

Part-of: <https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/4428>
@github-actions github-actions bot added the outdated PR was locked due to age label May 30, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
automerge-skip `brew pr-automerge` will skip this pull request CI-linux-self-hosted Build on Linux self-hosted runner CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. CI-published-bottle-commits The commits for the built bottles have been pushed to the PR branch. long build Set a long timeout for formula testing maintainer feedback Additional maintainers' opinions may be needed outdated PR was locked due to age ready to merge PR can be merged once CI is green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants