I see, thanks! Will think/investigate this more and return when having more
detailed proposal and justification
wtorek, 7 marca 2023 o 18:00:05 UTC+1 José Valim napisał(a):
> > Also, user doesn't have to think about versions as it includes only one
> dependency so it won't try to plug webrtc endpoint that is incompatible
> with engine.
>
> FWIW, this can be easily addressed by recommending users to add
> {:membrane_plugin, ">= 0.0.0"}, since membrane itself will already restrict
> it to a supported version.
>
> At the end of the day, if the goal is replacing 3 lines:
>
> {:membrane_rtc_engine_webrtc, ..}
> {:membrane_rtc_engine_hls, ...}
> {:membrane_rtc_engine_recorder, ...}
>
> with:
>
>
> {:membrane_rtc_engine, "~> 0.10.0", features: [:webrtc, :hls, :recorder]}
>
> I have to say this is likely not worth it, given the amount of work
> implementing and maintaining this feature would entail in the long term.
>
> I understand why Rust has this feature: compile-time and artefacts are
> much larger there. Plus the fact they can't metaprogram at the file level
> like us means they need explicit configuration for this. So before moving
> on, I think you need to send a more structured proposal, with the problem
> this is going to address, code snippets before and after, and so on. As
> mentioned above, this is a complex feature, so the benefits need to be
> really well justified over what can already be achieved today.
>
>
> On Tue, Mar 7, 2023 at 5:06 PM Michal Śledź <michal...@swmansion.com>
> wrote:
>
>> > I understand better now. To make the issue concise, you would like to
>> programmatically include optional dependencies. Today everything is based
>> on the dependency name itself but you would like to have an abstraction
>> layer for controlling it.
>>
>> Yes, exactly!
>>
>> > One potential workaround that you have is to define dependencies to
>> work as flags. Let's say you have a realtime feature that requires 3
>> dependencies, you can define an optional "membrane_realtime_feature"
>> package that, once included, signals the existance of a feature and also
>> all of its dependencies. Alghouth it is most likely not a good idea to
>> abuse dependencies in this way.
>>
>> That's what we were thinking about too. In general, we know in the
>> compile time which features we are going to need. At the end, user has to
>> plug specific endpoints to the Engine on its own. For example:
>>
>> {:ok, pid} = Membrane.RTC.Engine.start_link()
>> Engine.add_endpoint(pid, %HLS{})
>> Engine.add_endpoint(pid, %WebRTC{})
>> Engine.add_endpoint(pid, %VideoRecorder{})
>> etc.
>>
>> so in general, we could exctract each endpoint (WebRTC, HLS, Recorder)
>> into a separate package and I belive this is a pretty elegant solution.
>>
>> We would end up with:
>>
>> {:membrane_rtc_engine, ...}
>> {:membrane_rtc_engine_webrtc, ..}
>> {:membrane_rtc_engine_hls, ...}
>> {:membrane_rtc_engine_recorder, ...}
>>
>> However, allowing user to do
>>
>> {:membrane_rtc_engine, "~> 0.10.0", features: [:webrtc, :hls, :recorder]}
>>
>> looks even more attractive to me, and is easier to document as we can
>> list all of supported features in the membrane_rtc_engine docs. Also, user
>> doesn't have to think about versions as it includes only one dependency so
>> it won't try to plug webrtc endpoint that is incompatible with engine.
>>
>> Regarding:
>>
>> > I think the biggest concern with implementing this feature is that it
>> needs to be part of Hex itself. So the first discussion is if and how to
>> extend the Hex registry to incorporate this metadata, which needs to happen
>> in Hex first.
>>
>> and
>>
>> > In this case, as long as the feature checks are only inclusive, it
>> should be fine. But you can also think if a library named A assumes that
>> dependency C is compiled without some flag, and library B assumes C is
>> compiled with said flag, you will end up with conflicting behaviour.
>>
>> I don't have answers to those questions but I am willing to investigate
>> them and propose more detailed analysis on how we could implement the whole
>> concept assuming it sounds valid to you.
>>
>> wtorek, 7 marca 2023 o 16:34:50 UTC+1 José Valim napisał(a):
>>
>>> I understand better now. To make the issue concise, you would like to
>>> programmatically include optional dependencies. Today everything is based
>>> on the dependency name itself but you would like to have an abstraction
>>> layer for controlling it.
>>>
>>> I think the biggest concern with implementing this feature is that it
>>> needs to be part of Hex itself. So the first discussion is if and how to
>>> extend the Hex registry to incorporate this metadata, which needs to happen
>>> in Hex first.
>>>
>>> > I also thought that configuring libraries via Application environment
>>> is discouraged, according to
>>>
>>> https://hexdocs.pm/elixir/library-guidelines.html#avoid-application-configuration
>>>
>>> <https://hexdocs.pm/elixir/library-guidelines.html#avoid-application-configuration>
>>>
>>> Right. Application configuration has many downsides, exactly because it
>>> is global. The feature mechanism is also global, regardless if we put it on
>>> mix.exs or on the configuration environment. Rust also hints it is
>>> configuration (the conditional is called cfg):
>>>
>>> #[cfg(feature = "webp")]pub mod webp;
>>>
>>> In this case, as long as the feature checks are only inclusive, it
>>> should be fine. But you can also think if a library named A assumes that
>>> dependency C is compiled without some flag, and library B assumes C is
>>> compiled with said flag, you will end up with conflicting behaviour.
>>>
>>> ---
>>>
>>> One potential workaround that you have is to define dependencies to work
>>> as flags. Let's say you have a realtime feature that requires 3
>>> dependencies, you can define an optional "membrane_realtime_feature"
>>> package that, once included, signals the existance of a feature and also
>>> all of its dependencies. Alghouth it is most likely not a good idea to
>>> abuse dependencies in this way.
>>>
>>> On Tue, Mar 7, 2023 at 4:17 PM Michal Śledź <michal...@swmansion.com>
>>> wrote:
>>>
>>>> 2. Users of a library with optional dependencies have to include all
>>>> optional dependencies in their mix.exs
>>>> I meant that to enable one feature, user has to include a lot of
>>>> optional dependencies, at least in our case.
>>>>
>>>> 3. Users might include bad varsions of optional dependencies
>>>> Here, I meant that user has to exactly know dependency version that has
>>>> to be included. In our case, when there is a lot of optional dependencies,
>>>> it starts getting annoying to keep them up to date in the docs.
>>>>
>>>> Other than that, you should be able to provide this functionality using
>>>> config/config.exs files and the Application.compile_env/2.
>>>> But I cannot manipulate which deps should be downloaded and compiled
>>>> using Application.compile_env, can I? I mean, user still has to include
>>>> all
>>>> needed dependencies and know their correct versions. I also thought that
>>>> configuring libraries via Application environment is discouraged,
>>>> according
>>>> to
>>>>
>>>> https://hexdocs.pm/elixir/library-guidelines.html#avoid-application-configuration
>>>>
>>>> <https://hexdocs.pm/elixir/library-guidelines.html#avoid-application-configuration>
>>>>
>>>> We very often depend on native libraries written in C like ffmpeg. When
>>>> it's possible, we make those components optional, so that user is not
>>>> forced to install uneeded native libraries on their system.
>>>>
>>>> I feel like at the moment user has to be aware of which optional deps
>>>> are needed to get the desired feature. What I would like to have is to
>>>> focus on the feature itself, leaving deps and their versions to library
>>>> maintainers.
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> wtorek, 7 marca 2023 o 14:45:03 UTC+1 José Valim napisał(a):
>>>>
>>>>> Hi Michał,
>>>>>
>>>>> Thanks for the proposal. Your initial description makes me think there
>>>>> may exist bugs which we would need to investigate first.
>>>>>
>>>>> 2. Users of a library with optional dependencies have to include all
>>>>> optional dependencies in their mix.exs
>>>>>
>>>>> This should not be required. You only need to include the dependencies
>>>>> that you need, which would be equivalent to opting into a feature in Rust.
>>>>>
>>>>> 3. Users might include bad varsions of optional dependencies
>>>>>
>>>>> This should not be possible. The requirement has to match for optional
>>>>> dependencies.
>>>>>
>>>>> If the above is not true, please provide more context.
>>>>>
>>>>> ---
>>>>>
>>>>> Other than that, you should be able to provide this functionality
>>>>> using config/config.exs files and the Application.compile_env/2. In fact,
>>>>> I
>>>>> think introducing another mechanism to configure libraries could end-up
>>>>> adding more confusion, especially given how configs changed (and also
>>>>> improved) throughout the years.
>>>>>
>>>>>
>>>>>
>>>>> On Tue, Mar 7, 2023 at 12:40 PM Michal Śledź <michal...@swmansion.com>
>>>>> wrote:
>>>>>
>>>>>> Currently, using optional dependencies is quite inconvenient and
>>>>>> error prone:
>>>>>>
>>>>>> 1. A lot of modules have to use if Code.ensure_loaded statements
>>>>>> introducing additional nesting
>>>>>> 2. Users of a library with optional dependencies have to include all
>>>>>> optional dependencies in their mix.exs
>>>>>> 3. Users might include bad varsions of optional dependencies
>>>>>>
>>>>>> My proposal is to enhance API for optional dependencies basing on the
>>>>>> API provided by Cargo in Rust.
>>>>>>
>>>>>> The main idea is that the user of a library with optional
>>>>>> dependencies specify which "features" it is willing to have. For
>>>>>> example,
>>>>>> in membrane_rtc_engine library, which allows you to exchange audio/video
>>>>>> using different multimedia protocols, we have a lot of optional
>>>>>> dependencies depending on what protocol the user is willing to use. When
>>>>>> the user wants to receive media via webrtc and convert it to the HLS to
>>>>>> broadcast it to the broader audience it has to include all of those
>>>>>> dependencies
>>>>>>
>>>>>> # Optional deps for HLS endpoint
>>>>>> {:membrane_aac_plugin, "~> 0.13.0", optional: true},
>>>>>> {:membrane_opus_plugin, "~> 0.16.0", optional: true},
>>>>>> {:membrane_aac_fdk_plugin, "~> 0.14.0", optional: true},
>>>>>> {:membrane_generator_plugin, "~> 0.8.0", optional: true},
>>>>>> {:membrane_realtimer_plugin, "~> 0.6.0", optional: true},
>>>>>> {:membrane_audio_mix_plugin, "~> 0.12.0", optional: true},
>>>>>> {:membrane_raw_audio_format, "~> 0.10.0", optional: true},
>>>>>> {:membrane_h264_ffmpeg_plugin, "~> 0.25.2", optional: true},
>>>>>> {:membrane_audio_filler_plugin, "~> 0.1.0", optional: true},
>>>>>> {:membrane_video_compositor_plugin, "~> 0.2.1", optional: true},
>>>>>> {:membrane_http_adaptive_stream_plugin, "~> 0.11.0", optional:
>>>>>> true},
>>>>>>
>>>>>> Instead of this, I would love to say to the user, hi if you want to
>>>>>> use HLS just specify it in the feature list. For example:
>>>>>>
>>>>>> {:membrane_rtc_engine, "~> 0.10.0", features: [:hls]}
>>>>>>
>>>>>> It would also be nice to somehow get rid of "if Code.ensure_loaded"
>>>>>> statements. I am not sure how yet but Rust do this that way
>>>>>>
>>>>>> // This conditionally includes a module which implements WEBP support.
>>>>>> #[cfg(feature
>>>>>> = "webp")] pub mod webp;
>>>>>>
>>>>>> What comes to my mind is that in mix.exs we can specify "features",
>>>>>> their dependencies and a list of modules. When someone asks for the
>>>>>> feature, those dependencies are autmatically downloaded and listed
>>>>>> modules
>>>>>> are compiled.
>>>>>>
>>>>>> The final proposal is:
>>>>>>
>>>>>> # library side
>>>>>> # mix.exs
>>>>>>
>>>>>> features: [
>>>>>> hls: [
>>>>>> dependencies: [],
>>>>>> modules: []
>>>>>> ]
>>>>>> ]
>>>>>>
>>>>>> # user side
>>>>>> # mix.exs
>>>>>>
>>>>>> {:membrane_rtc_engine, "~> 0.10.0", features: [:hls]}
>>>>>>
>>>>>> I would love to help in implementing those features if you decide
>>>>>> they are valuable
>>>>>>
>>>>>> Rust reference:
>>>>>> https://doc.rust-lang.org/cargo/reference/features.html#features
>>>>>>
>>>>>>
>>>>>> --
>>>>>> You received this message because you are subscribed to the Google
>>>>>> Groups "elixir-lang-core" group.
>>>>>> To unsubscribe from this group and stop receiving emails from it,
>>>>>> send an email to elixir-lang-co...@googlegroups.com.
>>>>>> To view this discussion on the web visit
>>>>>> https://groups.google.com/d/msgid/elixir-lang-core/8385965e-799d-4cea-bcd5-151d9fee6914n%40googlegroups.com
>>>>>>
>>>>>> <https://groups.google.com/d/msgid/elixir-lang-core/8385965e-799d-4cea-bcd5-151d9fee6914n%40googlegroups.com?utm_medium=email&utm_source=footer>
>>>>>> .
>>>>>>
>>>>> --
>>>> You received this message because you are subscribed to the Google
>>>> Groups "elixir-lang-core" group.
>>>> To unsubscribe from this group and stop receiving emails from it, send
>>>> an email to elixir-lang-co...@googlegroups.com.
>>>>
>>> To view this discussion on the web visit
>>>> https://groups.google.com/d/msgid/elixir-lang-core/d88a5141-86d8-42b8-ae61-71cf58d644a0n%40googlegroups.com
>>>>
>>>> <https://groups.google.com/d/msgid/elixir-lang-core/d88a5141-86d8-42b8-ae61-71cf58d644a0n%40googlegroups.com?utm_medium=email&utm_source=footer>
>>>> .
>>>>
>>> --
>> You received this message because you are subscribed to the Google Groups
>> "elixir-lang-core" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to elixir-lang-co...@googlegroups.com.
>>
> To view this discussion on the web visit
>> https://groups.google.com/d/msgid/elixir-lang-core/a0f6ba5c-1a50-4637-90c9-c3968b443377n%40googlegroups.com
>>
>> <https://groups.google.com/d/msgid/elixir-lang-core/a0f6ba5c-1a50-4637-90c9-c3968b443377n%40googlegroups.com?utm_medium=email&utm_source=footer>
>> .
>>
>
--
You received this message because you are subscribed to the Google Groups
"elixir-lang-core" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to elixir-lang-core+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/elixir-lang-core/a1733e94-9cee-4a54-8e86-ce1da05e25a9n%40googlegroups.com.
