Precompiled deps #107

varsill · 2023-08-07T08:02:34Z

closes membraneframework/membrane_core#603
This PR adds an ability to specify precompiled dependencies.
Its usage can be seen here: membraneframework/membrane_h264_ffmpeg_plugin#92

… should be downloaded. Set the rpath for the created dynamic libraries

…deps (with the use of pkgconfig and precompiled deps). Add Bundlex.OSDeps module.

…e debug prints

…new-dtags only in GCC

mat-hek · 2023-08-31T11:54:52Z

lib/bundlex/os_deps.ex

+        :curl -> "curl --fail -L -o #{dest} #{url}"
+        :wget -> "wget -O #{dest} #{url}"
+      end


let's use an elixir/erlang library for that, for example https://hexdocs.pm/req/

mat-hek · 2023-08-31T11:55:44Z

lib/bundlex/precompiled_dependency.ex

@@ -0,0 +1,69 @@
+defmodule Bundlex.PrecompiledDependency do


As we spoke, let's remove the behaviour in favour of accepting config

as discussed, I will do it later if we assume that this approach is still valid

The behaviour has been removed ;)

mat-hek · 2023-08-31T11:57:13Z

test/bundlex/integration_test.exs

-    assert {_output, 0} = System.cmd("sh", ["-c", "mix test 1>&2"], cd: "test_projects/example")
+    System.cmd("sh", ["-c", "mix test 1>&2"], cd: "test_projects/example")


why remove the assertion?

I have revoked it ;)

mat-hek · 2023-08-31T13:08:07Z

lib/bundlex/os_deps.ex

+  require Logger
+  alias Bundlex.{Output, PrecompiledDependency}
+
+  @precompiled_path "_build/#{Mix.env()}/precompiled/"


use Mix.Project.build_path() and name the dir with prefix, e.g. 'bundlex_precompiled'

mat-hek · 2023-08-31T13:10:24Z

lib/bundlex/native.ex

-      Map.take(dependency, [:includes, :libs, :lib_dirs, :pkg_configs, :linker_flags, :deps]),
+      Map.take(dependency, [:includes, :libs, :lib_dirs, :os_deps, :linker_flags, :deps]),


unless we don't want to release a new version, we should still support pkg_configs, just deprecate them

mat-hek · 2023-08-31T13:38:42Z

lib/bundlex/os_deps.ex

+  defp remove_lib_prefix(libname) do
+    if String.starts_with?(libname, "lib") do
+      String.slice(libname, 3..-1)
+    else
+      libname
+    end
+  end


Suggested change

defp remove_lib_prefix(libname) do

if String.starts_with?(libname, "lib") do

String.slice(libname, 3..-1)

else

libname

end

end

defp remove_lib_prefix("lib" <> libname), do: libname

defp remove_lib_prefix(libname), do: libname

good idea, thanks :D

mat-hek · 2023-09-01T11:12:30Z

lib/bundlex/os_deps.ex

@@ -0,0 +1,204 @@
+defmodule Bundlex.OSDeps do


I'm thinking about moving this to Budlex.Toolchain.Common.Unix.OsDeps, as it's quite unix-specific, though the name would get scaringly java-style :P

Ok, I will move it ;)

lib/bundlex/toolchain/common/unix/os_deps.ex

mat-hek · 2023-09-05T10:08:40Z

lib/bundlex.ex

+  It consists of:
+  * architecture - e.g. `x86_64` or `arm64`
+  * vendor - e.g. `pc`
+  * operating system - e.g. `linux` or `freebsd`
+  """
+  @type target_triplet ::
+          {architecture :: String.t(), vendor :: String.t(), operating_system :: String.t()}


I'd make it a struct for more flexibility and easier access to fields. Also, maybe we could put the version of the operating system in a separate field, so we didn't have to pattern match it. I'd consider converting strings to atoms as well.

I am not sure about putting the operating system's version as a separate field - we would need to be aware of all possible values of the operating system returned by the :erlang.system_info, since some of them might be just a system name (e.g. linux), and some of them contain the version (e.g. darwin20.6.0).
For a person who needs to use the os value (that is a person who prepares the precompiled dependency) it's not that much effort to pattern match on something like "darwin"<>_version since it's already aware what the os string look like on a given system.

That is also why I wouldn't convert the names into atoms, as it will make such a pattern matching more difficult (or impossible)

Hmm, so maybe we could have a function like Bundlex.family() that would convert the OS to one of the well-known values? Then we could gradually improve it to support more systems if needed

mat-hek · 2023-09-05T10:24:40Z

test_projects/example/bundlex.exs

-          {PrecompiledFFmpeg, [:libswscale, :libavcodec]},
-          :libpng
-        ]
+          {get_ffmpeg_path(), ["libswscale", "libavcodec"]},


I thought about this implicit pkg-config as a default, and maybe we could accept a list of 'providers', which could be either {:precompiled, url} or :pkg_config or nil. So in this case we'd have

Suggested change

{get_ffmpeg_path(), ["libswscale", "libavcodec"]},

{[get_ffmpeg_path(), :pkg_config], ["libswscale", "libavcodec"]},

And get_ffmpeg_path() would return {:precompiled, url} or nil. This way is more verbose but less implicit and the user can decide whether to take the precompiled or OS package first ;) WDYT?

lib/bundlex/toolchain/common/unix/os_deps.ex

mat-hek · 2023-09-05T10:28:21Z

lib/bundlex/toolchain/common/unix/os_deps.ex

@@ -152,7 +136,7 @@ defmodule Bundlex.OSDeps do
        try do
          File.mkdir(package_path)
          temporary_destination = "#{@precompiled_path}/temporary"
-          download(url, temporary_destination)
+          download(precompiled_dependency_url, temporary_destination)
          System.shell("tar -xf #{temporary_destination} -C #{package_path} --strip-components 1")
          System.shell("rm #{temporary_destination}")


Suggested change

System.shell("rm #{temporary_destination}")

File.rm!(temporary_destination)

mat-hek · 2023-09-05T10:31:24Z

lib/bundlex/toolchain/common/unix/os_deps.ex

@@ -173,32 +157,19 @@ defmodule Bundlex.OSDeps do
      String.split(url, "/")
      |> Enum.at(-1)
      |> String.split(".")
-      |> Enum.reject(&(&1 in ["tar", "xz"]))
+      |> Enum.reject(&(&1 in ["tar", "xz", "gz"]))


I've bumped into https://github.com/ricn/zarex, maybe it's worth considering?

good idea, done

mat-hek · 2023-09-05T10:32:10Z

lib/bundlex/toolchain/common/unix/os_deps.ex

-    case System.shell(command) do
-      {_, 0} -> :ok
-      _other -> :error
+    if response.status == 200 do


nit: case & pattern match would look better

mat-hek · 2023-09-05T10:34:46Z

lib/bundlex/toolchain/common/unix/os_deps.ex

+  require Logger
+  alias Bundlex.Output
+
+  @precompiled_path "#{Mix.Project.build_path()}/bundlex_precompiled/"


I'm not sure how this path can change, so I'd make it defp just in case

README.md

mat-hek · 2023-09-11T08:39:55Z

lib/bundlex/toolchain/common/unix/os_deps.ex

+    []
+  end
+
+  defp resolve_single_os_dep(providers, lib_names) do


As discussed, you can resolve all the flags and paths here. If you add them to existing fields in Native, there'll be no need for the resolved_os_deps field.

mat-hek · 2023-09-11T08:41:23Z

lib/bundlex/toolchain/common/unix/os_deps.ex

-              {{precompiled_dependency, package_path}, lib_names}
-          end
-      end)
+  # defp parse_os_deps(os_deps) do


lib/bundlex.ex

lib/bundlex/toolchain/common/unix/os_deps.ex

FelonEkonom · 2023-09-11T12:16:17Z

lib/bundlex/toolchain/common/unix/os_deps.ex

        try do
-          File.mkdir(package_path)
-          temporary_destination = "#{@precompiled_path}/temporary"
+          File.mkdir!(package_path)
+          temporary_destination = "#{get_precompiled_path()}/temporary"
          download(precompiled_dependency_url, temporary_destination)
-          System.shell("tar -xf #{temporary_destination} -C #{package_path} --strip-components 1")
-          System.shell("rm #{temporary_destination}")
+
+          {_output, 0} =
+            System.shell(
+              "tar -xf #{temporary_destination} -C #{package_path} --strip-components 1"
+            )
+
+          File.rm!(temporary_destination)
          package_path
        rescue
          e ->
-            Logger.warning("Couldn't download the dependency due to: #{inspect(e)}.")
+            IO.warn("Couldn't download the dependency due to: #{inspect(e)}.")
            :unavailable
        end


This try shouldn't rescue exception from File.mkdir!/1 function

BTW, why do we need try here?

Because there is a possibility that in example, due to network conditions, the dependency won't be downloaded

mat-hek · 2023-09-19T11:49:26Z

README.md

@@ -128,6 +128,17 @@ Configuration of each native may contain following options:
 * `libs` - Names of libraries to link (empty list by default).
 * `pkg_configs` - (deprecated) Names of libraries for which the appropriate flags will be
 obtained using pkg-config (empty list by default).
+* `os_deps` - List of external OS dependencies. It's a list of elements in form of 


Suggested change

* `os_deps` - List of external OS dependencies. It's a list of elements in form of

* `os_deps` - List of external OS dependencies. It's a list of tuples in the form of

mat-hek · 2023-09-19T11:54:46Z

lib/bundlex/toolchain/common/unix/os_deps.ex

+    compiler_flags =
+      Enum.uniq(cflags_list)
+      |> Enum.join(" ")
+
+    libs_flags =
+      Enum.uniq(libs_list)
+      |> Enum.join(" ")


can't these be just concatenated with respective native.*_flags?

I have slightly modified it

mat-hek · 2023-09-19T14:16:23Z

lib/bundlex/project.ex

@@ -40,6 +40,7 @@ defmodule Bundlex.Project do
          includes: [String.t()],
          lib_dirs: [String.t()],
          libs: [String.t()],
+          os_deps: [Bundlex.Native.os_dep()],


It's not documented in the typedoc. Right now, this typedoc is copied to readme so we have to update both places. Maybe we can replace it in the readme with an example and a link to the typedoc?

mat-hek · 2023-09-19T14:36:04Z

lib/bundlex/toolchain/common/unix/os_deps.ex

+             get_flags_for_pkg_config(lib_names, :libs, app)}
+          ]
+        rescue
+          _e ->


Here we silence any error that may have happened. I'd print it, otherwise, there's no way to see what went wrong.

Or we can print something like 'Failed to fetch the OS libs X, Y, Z via pkg-config, trying other ways' and print the original error(s) only if no provider succeeds

mat-hek · 2023-09-19T14:36:48Z

lib/bundlex/toolchain/common/unix/os_deps.ex

+  end
+
+  defp resolve_single_os_dep([], lib_names, _app) do
+    IO.warn("Couldn't load OS dependencies for libraries: #{inspect(lib_names)}.")


I think we should raise here

FelonEkonom · 2023-09-20T11:50:25Z

lib/bundlex/toolchain/common/unix/os_deps.ex

-  # defp parse_os_deps(os_deps) do
-  #   Enum.map(os_deps, fn
-  #     {:precompiled, precompiled_dependency_url, lib_name_or_names} ->
-  #       lib_names = Bunch.listify(lib_name_or_names)
-  #       {precompiled_dependency_url, lib_names}
-
-  #     lib_name ->
-  #       {:pkg_config, lib_name}
-  #   end)
-  # end


FelonEkonom · 2023-09-20T11:54:57Z

lib/bundlex/toolchain/common/unix/os_deps.ex

        try do
-          File.mkdir!(package_path)
          temporary_destination = "#{get_precompiled_path()}/temporary"
          download(precompiled_dependency_url, temporary_destination)



IO.warn in rescuebelow will print a misleading warning in case of error thrown from System.shell or File.rm!

FelonEkonom · 2023-09-20T12:00:08Z

lib/bundlex/toolchain/common/unix/os_deps.ex

-
-      true ->
-        raise "Unknown flag type: #{inspect(flags_type)}"
+        "-I#{full_include_path}"


Suggested change

"-I#{full_include_path}"

[ "-I#{full_include_path}"]

Not having a list here might cause an error thrown from Enum.zip at line 17

mat-hek · 2023-10-02T09:27:52Z

lib/bundlex/toolchain/common/unix/os_deps.ex

        providers = Bunch.listify(provider_or_providers)
        lib_names = Bunch.listify(lib_name_or_names)

        resolve_single_os_dep(providers, lib_names, native.app)
      end)
      |> Enum.unzip()
+      |> then(&{List.flatten(elem(&1, 0)), List.flatten(elem(&1, 1))})


This is hard to read, let's do that below, alongside Enum.uniq

lib/bundlex/build_script.ex

mat-hek · 2023-10-02T12:04:57Z

lib/bundlex/toolchain/common/unix/os_deps.ex

    compiler_flags =
-      Enum.uniq(cflags_list)
+      List.flatten(cflags_list)
+      |> Enum.uniq()


Suggested change

compiler_flags =

Enum.uniq(cflags_list)

List.flatten(cflags_list)

|> Enum.uniq()

compiler_flags = cflags_list |> List.flatten() |> Enum.uniq()

mat-hek · 2023-10-02T12:05:47Z

lib/bundlex/toolchain/common/unix/os_deps.ex

    libs_flags =
-      Enum.uniq(libs_list)
+      List.flatten(libs_list)
+      |> Enum.uniq()


Suggested change

libs_flags =

Enum.uniq(libs_list)

List.flatten(libs_list)

|> Enum.uniq()

libs_flags = libs_list |> List.flatten() |> Enum.uniq()

varsill added 11 commits August 3, 2023 16:47

WIP

791a9ce

Implement POC

0c0cc52

Refactor the code. Add Bundlex.Precompiled module

3de5dc7

Add API to specify precompiled dependencies repository

04b8311

Allow to specify the repository from which the precompiled dependency…

7c1b73b

… should be downloaded. Set the rpath for the created dynamic libraries

Refactor the code. Improve and unify the API for specification of os …

8d151d5

…deps (with the use of pkgconfig and precompiled deps). Add Bundlex.OSDeps module.

Repair the test

153dba6

Remove redundant joining

73b23bd

Add documentation and typespecs

2971e56

Save the package path after the precompiled dependency is downloaded

f7e3856

Fix a bug with get_package_path failing for :unavailable input. Remov…

547c67d

…e debug prints

varsill marked this pull request as ready for review August 30, 2023 12:17

varsill requested a review from FelonEkonom as a code owner August 30, 2023 12:17

varsill requested a review from mat-hek August 30, 2023 12:17

varsill added 4 commits August 30, 2023 14:21

Remove unused variable

ca23050

Format the code

44ad467

Make credo happy

09b9316

Update the bundlex test with new precompiled deps API. Use --disable-…

da40f4d

…new-dtags only in GCC

mat-hek requested changes Sep 1, 2023

View reviewed changes

varsill added 3 commits September 4, 2023 11:39

Implement reviewers suggestions.

f07ba87

Make os_deps accept lib names in form of strings

03d1b68

Update mix.lock

e4a04ef

varsill requested a review from mat-hek September 4, 2023 11:45

varsill added 2 commits September 4, 2023 16:15

Get rid of PrecompiledDependency behaviour

92fa45f

Add get_target()

723110c

FelonEkonom requested changes Sep 5, 2023

View reviewed changes

mat-hek requested changes Sep 5, 2023

View reviewed changes

varsill added 3 commits September 7, 2023 14:44

Implement reviewers suggestions.

d4608b4

get rid of do_get_flags_for_pkg_config

1003434

Fix dialyzer

5f970c4

varsill requested review from FelonEkonom and mat-hek September 8, 2023 14:29

mat-hek requested changes Sep 11, 2023

View reviewed changes

FelonEkonom requested changes Sep 11, 2023

View reviewed changes

varsill added 2 commits September 19, 2023 11:38

Implement reviewers suggestions.

a7c4b47

Remove os_resolved_deps

2cc088d

varsill requested review from mat-hek and FelonEkonom September 19, 2023 11:23

make dialyzer happy

afc0a75

mat-hek requested changes Sep 19, 2023

View reviewed changes

varsill added 2 commits September 19, 2023 17:22

Fix a bug

e6c925d

Implement reviewers suggestions.

370a9e0

FelonEkonom requested changes Sep 20, 2023

View reviewed changes

Noarkhh added 2 commits September 26, 2023 10:46

Fix unix compile command

b4557f1

Stop warning

a18ddd3

michael-bieszczad mentioned this pull request Sep 28, 2023

Precompilation support in Bundlex membraneframework/membrane_mp3_mad_plugin#38

Closed

Apply code review suggestions

430f180

Noarkhh requested a review from FelonEkonom September 29, 2023 12:34

Fix stderr_to_stdout

74c21a8

mat-hek reviewed Oct 2, 2023

View reviewed changes

FelonEkonom reviewed Oct 2, 2023

View reviewed changes

lib/bundlex/build_script.ex Outdated Show resolved Hide resolved

FelonEkonom self-requested a review October 2, 2023 10:25

FelonEkonom approved these changes Oct 2, 2023

View reviewed changes

Update example for test

fc67e2f

mat-hek approved these changes Oct 2, 2023

View reviewed changes

Noarkhh added 3 commits October 2, 2023 14:53

Add -Wno-unused-function flag

5c5a89d

Try not inhibiting warnings

91c559b

Inhibit all warnings

e56a0a4

Noarkhh merged commit 27a8513 into master Oct 5, 2023
3 checks passed

Noarkhh deleted the precompiled_deps branch October 5, 2023 14:18

		assert {_output, 0} = System.cmd("sh", ["-c", "mix test 1>&2"], cd: "test_projects/example")
		System.cmd("sh", ["-c", "mix test 1>&2"], cd: "test_projects/example")

		Map.take(dependency, [:includes, :libs, :lib_dirs, :pkg_configs, :linker_flags, :deps]),
		Map.take(dependency, [:includes, :libs, :lib_dirs, :os_deps, :linker_flags, :deps]),

	{get_ffmpeg_path(), ["libswscale", "libavcodec"]},
	{[get_ffmpeg_path(), :pkg_config], ["libswscale", "libavcodec"]},

	System.shell("rm #{temporary_destination}")
	File.rm!(temporary_destination)

	* `os_deps` - List of external OS dependencies. It's a list of elements in form of
	* `os_deps` - List of external OS dependencies. It's a list of tuples in the form of

Precompiled deps #107

Precompiled deps #107

Conversation

varsill commented Aug 7, 2023 • edited by Noarkhh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mat-hek Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FelonEkonom Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varsill commented Aug 7, 2023 •

edited by Noarkhh

Loading

mat-hek Sep 19, 2023 •

edited

Loading

FelonEkonom Sep 20, 2023 •

edited

Loading