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

Adjust to precompiled dependencies API of bundlex #92

Merged
merged 27 commits into from
Oct 6, 2023

Conversation

varsill
Copy link
Contributor

@varsill varsill commented Aug 30, 2023

This PR adjust the bundlex configuration to the new API allowing for precompiled specification.

@varsill varsill marked this pull request as ready for review August 30, 2023 12:38
@varsill varsill requested a review from mat-hek August 30, 2023 12:38
bundlex.exs Outdated
Comment on lines 28 to 30
os_deps: [
{get_ffmpeg(), ["libavcodec", "libswresample", "libavutil"]}
],
Copy link
Member

@FelonEkonom FelonEkonom Sep 4, 2023

Choose a reason for hiding this comment

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

Instead of having get_ffmpeg/0 function, create a function like

defp ffmpeg_os_deps(), do: [{get_ffmpeg(), ["libavcodec", "libswresample", "libavutil"]}]

to avoid repeating yourself.

If you decide to still have a function with body of get_ffmpeg/0, I would suggest to rename it to something like get_ffmpeg_url/0

mix.exs Outdated
{:unifex, "~> 1.1"},
{:membrane_core, "~> 0.12.0"},
{:membrane_core, "~> 0.12.7"},
Copy link
Member

Choose a reason for hiding this comment

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

We have already released v0.12.8 of membrane_core 😜

bundlex.exs Outdated
@@ -12,19 +27,25 @@ defmodule Membrane.H264.FFmpeg.BundlexProject do
parser: [
interface: :nif,
sources: ["parser.c"],
pkg_configs: ["libavcodec", "libavutil"],
os_deps: [
{[get_ffmpeg(), :pkg_config], ["libavcodec", "libswresample", "libavutil"]}
Copy link
Member

Choose a reason for hiding this comment

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

do we need swresample?

Copy link
Contributor Author

@varsill varsill left a comment

Choose a reason for hiding this comment

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

@Noarkhh - LGTM 🥇 (but I cannot approve it as I am the one who has opened the PR :D)

@Noarkhh Noarkhh merged commit c6265cc into master Oct 6, 2023
3 checks passed
@Noarkhh Noarkhh deleted the use_precompiled_deps branch October 6, 2023 13:39
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.

None yet

4 participants