627 support for streaming via stdio #50
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Weird finding while fixing the MIX_ENV bug: root@d326e438f233:~/project# mix test
Generated membrane_file_plugin app
..............................
1) test pipeline from :stdin to file works when content is longer than chunk_size (Membrane.File.Integration.StdioTest)
test/integration/stdio_test.exs:37
Assertion with == failed
code: assert {"", _rc = 0} ==
System.shell(
"bash -c ' set -o pipefail; cat #{@input_text_file} | mix run test/fixtures/pipe_to_file.exs #{cmd_out} 5' 2> #{cmd_err}",
env: [{"MIX_QUIET", "true"}, {"MIX_ENV", "dev"}]
)
left: {"", 0}
right: {"src/parser.yrl: Warning: conflicts: 27 shift/reduce, 0 reduce/reduce\n", 0}
stacktrace:
test/integration/stdio_test.exs:39: (test)
...
Finished in 47.1 seconds (0.5s async, 46.6s sync)
34 tests, 1 failure
Randomized with seed 913776
root@d326e438f233:~/project# mix test
..................................
Finished in 8.8 seconds (0.4s async, 8.4s sync)
34 tests, 0 failures
Randomized with seed 246311 |
varsill
requested changes
Jan 10, 2024
|
Great! I have left some doubts and suggestions in the comments ;) Apart from that, as far as I can see, one of the new tests is failing on the CI |
varsill
reviewed
Jan 16, 2024
varsill
reviewed
Jan 16, 2024
mat-hek
requested changes
Jan 16, 2024
There was a problem hiding this comment.
This requires to be run within a mix project. Let's add Mix.install so it can be run with elixir file_to_pipe.exs
examples/file_to_pipe.exs
Outdated
| @@ -0,0 +1,19 @@ | |||
| import Membrane.ChildrenSpec | |||
| import Membrane.Testing.Assertions | |||
| alias Membrane.Testing.Pipeline | |||
There was a problem hiding this comment.
This is intended to be used in tests, let's use a regular pipeline or Membrane.RCPipeline instead
mat-hek
requested changes
Jan 19, 2024
examples/file_to_pipe.exs
Outdated
Comment on lines
14
to
32
| # def yes?(_message, _opts \\ []), do: :ok | ||
|
|
||
| # def cmd(command, opts \\ []) do | ||
| # Mix.Shell.cmd(command, opts, fn data -> data end) | ||
| # end | ||
| # end | ||
|
|
||
| # {:ok, config} = :logger.get_handler_config(:default) | ||
| # config_dev = put_in(config, [:config, :type], :standard_error) | ||
|
|
||
| # :ok = | ||
| # :logger.set_handler_config(:default, put_in(config, [:config, :type], :standard_error)) | ||
|
|
||
| # :logger.remove_handler(:default) | ||
| # :ok = | ||
| # :logger.add_handler(:default, :logger_std_h, put_in(config, [:config, :type], :standard_error)) | ||
|
|
||
| # Logger.configure(device: :standard_error) | ||
| # Logger.Backends.Internal.configure(device: :standard_error) |
There was a problem hiding this comment.
Are you sure this is finished? :P
mat-hek
approved these changes
Jan 30, 2024
Co-authored-by: Mateusz Front <[email protected]>
varsill
reviewed
Feb 5, 2024
| # currently also requires 'configure: :logger, backends: []' line in config.exs | ||
| LoggerBackends.add(LoggerBackends.Console) | ||
| LoggerBackends.configure(LoggerBackends.Console, device: :standard_error) | ||
| Mix.install([{:membrane_file_plugin, path: "."}]) |
There was a problem hiding this comment.
shouldn't it be path: "../"? I believe we are in the examples/ subdirectory
mix.exs
Outdated
Comment on lines
82
to
113
| # Also, a small bug noticed when reproducing on elixir 1.16.0, this time related to mix: | ||
| # ```elixir | ||
| # require Logger | ||
|
|
||
| # Mix.start() | ||
| # Mix.shell(Mix.Shell.Quiet) | ||
|
|
||
| # Mix.install( | ||
| # [ | ||
| # :ratio, | ||
| # :logger_backends | ||
| # ], | ||
| # force: true | ||
| # ) | ||
| # ``` | ||
| # ```command | ||
| # root@5da670a83b2e:/workspace/membrane/logger_mre# elixirc mre.exs 2> stderr.log | ||
| # ==> ratio | ||
| # ``` | ||
| # stderr.log: | ||
| # ```log | ||
| # Every 2.0s: cat stderr.log 5da670a83b2e: Tue Jan 30 12:35:47 2024 | ||
|
|
||
| # warning: Ratio.DecimalConversion.decimal_to_rational/1 is undefined (module Ratio.DecimalConversion is not availabl | ||
| # e or is yet to be defined) | ||
| # │ | ||
| # 17 │ {ratio, Ratio.DecimalConversion.decimal_to_rational(decimal)} | ||
| # │ ~ | ||
| # │ | ||
| # └─ lib/ratio/coerce.ex:17:37: Coerce.Implementations.Ratio.Decimal.coerce/2 | ||
|
|
||
| # both :extra_applications and :applications was found in your mix.exs. You most likely want to remove the :applications |
There was a problem hiding this comment.
I think we can remove this one (I believe it's good to describe such problematic cases for instance in the PR's description).
However, we can warn user in the example description, that in case there are some warnings thrown during the compilation, the example might not work.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.