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

Improve precompiled DX #121

Merged
merged 4 commits into from
Jan 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/bundlex/build_script.ex
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ defmodule Bundlex.BuildScript do

use Bunch

alias Bundlex.Platform
alias Bundlex.{Output, Platform}

@script_ext unix: ".sh", windows: ".bat"
@script_prefix unix: "#!/bin/sh\n", windows: "@echo off\r\n"
Expand All @@ -29,7 +29,7 @@ defmodule Bundlex.BuildScript do
def run(script, platform)

def run(%__MODULE__{commands: []}, _platform) do
IO.warn("The list of commands is empty, did you forget to specify natives?")
Output.warn("The list of commands is empty, did you forget to specify natives?")
end

def run(%__MODULE__{commands: commands}, platform) do
Expand Down
8 changes: 6 additions & 2 deletions lib/bundlex/loader.ex
Original file line number Diff line number Diff line change
Expand Up @@ -140,11 +140,15 @@ defmodule Bundlex.Loader do
:ok
else
{:error, {reason, text}} ->
raise """
require Logger

Logger.error("""
Comment on lines +143 to +145
Copy link
Member

@FelonEkonom FelonEkonom Jan 8, 2024

Choose a reason for hiding this comment

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

Suggested change
require Logger
Logger.error("""
Bundlex.Output.warn("""

Are you sure Logger will work here? Loading a NIF may happen in the compile time

Copy link
Member Author

@mat-hek mat-hek Jan 8, 2024

Choose a reason for hiding this comment

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

I'm quite sure. I changed it from raise because previously we'd get both error and warning, both via Logger. I think Bundlex.Output.warn would work too, but I'm not sure about other Bundlex.Output functions, so I'd not use it in runtime

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, so I checked and in the compilation the bundlex compilation is called first and there we do Application.ensure_all_started https://github.com/membraneframework/bundlex/blob/master/lib/mix/tasks/compile.bundlex.ex#L22 and because we have :logger in extra_applications in mix.exs, the logger should be started at that point. So, it seems that the logger is started before any C code is even compiled.

Copy link
Member

Choose a reason for hiding this comment

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

If so, is there any reason to call IO.warn/1 in Bundlex.Output.warn/1 or Mix.shell().info/1 in Bundlex.Output.info/1?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know why we do this, probably we could use Logger as well ¯\_(ツ)_/¯

Bundlex cannot load nif #{inspect(nif_name)} of app #{inspect(app)}
from "#{path}", check bundlex.exs file for information about nifs.
Reason: #{inspect(reason)}, #{to_string(text)}
"""
""")

:abort
end
end
end
Expand Down
2 changes: 1 addition & 1 deletion lib/bundlex/native.ex
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ defmodule Bundlex.Native do

native =
if native.pkg_configs != [] do
IO.warn("`pkg_configs` option has been deprecated. Please use `os_deps` option.")
Output.warn("`pkg_configs` option has been deprecated. Please use `os_deps` option.")
%{native | os_deps: [{:pkg_config, native.pkg_configs} | native.os_deps]}
else
native
Expand Down
5 changes: 5 additions & 0 deletions lib/bundlex/output.ex
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,11 @@ defmodule Bundlex.Output do
Mix.shell().info("Bundlex: " <> msg)
end

@spec warn(String.t()) :: :ok
def warn(msg) do
IO.warn("Bundlex: " <> msg, [])
end

@spec raise(binary()) :: no_return()
def raise(msg) do
Mix.raise("Bundlex: " <> msg)
Expand Down
9 changes: 7 additions & 2 deletions lib/bundlex/platform.ex
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,9 @@ defmodule Bundlex.Platform do
Otherwise raises Mix error.
"""
@spec get_target!() :: name_t
if Mix.target() == :host do
mix_target = Mix.target()

if mix_target == :host do
def get_target!(), do: get_host!()
else
def get_target!() do
Expand All @@ -50,7 +52,10 @@ defmodule Bundlex.Platform do
:nerves

:error ->
IO.warn("Crosscompilation supported only for Nerves systems, assuming host's platform")
Output.warn(
"MIX_TARGET #{inspect(unquote(mix_target))} is not supported. Bundlex will compile for the host platform."
)

get_host!()
end
end
Expand Down
20 changes: 13 additions & 7 deletions lib/bundlex/project.ex
Original file line number Diff line number Diff line change
Expand Up @@ -47,15 +47,21 @@ defmodule Bundlex.Project do
- `precompiled` - Downloads the dependency from a given url and sets appropriate compilation
and linking flags. Can be either `{:precompiled, url, libs}` or `{:precompiled, url}`, in which
case the dependency name will be used as the lib name.
Precompiled dependencies for given applications (Mix projects) can be disabled via configuration, for example:
Precompiled dependencies can be disabled via configuration globally:

```elixir
config :bundlex, :disable_precompiled_os_deps,
apps: [:my_application, :another_application]
```
```elixir
config :bundlex, :disable_precompiled_os_deps, true
```

Note that this will affect the natives and libs defined in the `bundlex.exs` files of specified
applications only, not in their dependencies.
or for given applications (Mix projects), for example:

```elixir
config :bundlex, :disable_precompiled_os_deps,
apps: [:my_application, :another_application]
```

Note that this will affect the natives and libs defined in the `bundlex.exs` files of specified
applications only, not in their dependencies.
Comment on lines +52 to +64
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary additional tab

Copy link
Member Author

Choose a reason for hiding this comment

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

Without it the indent breaks in the generated docs for some reason


Check `t:os_dep/0` for details.
* `pkg_configs` - (deprecated, use `os_deps` instead) Names of libraries for which the appropriate flags will be
Expand Down
39 changes: 22 additions & 17 deletions lib/bundlex/toolchain/common/unix/os_deps.ex
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ defmodule Bundlex.Toolchain.Common.Unix.OSDeps do
end

if is_old_api do
IO.warn("""
Output.warn("""
Native #{inspect(native_name)} uses deprecated syntax for `os_deps`. \
See `Bundlex.Project.os_dep` for the new syntax.
""")
Expand Down Expand Up @@ -110,12 +110,13 @@ defmodule Bundlex.Toolchain.Common.Unix.OSDeps do
end

defp resolve_os_dep_provider(name, {:precompiled, url, lib_names}, native) do
disabled_apps =
Application.get_env(:bundlex, :disable_precompiled_os_deps, [])
|> Keyword.get(:apps, [])
|> Bunch.listify()
is_precompiled_disabled =
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
is_precompiled_disabled =
precompiled_disabled? =

Copy link
Member Author

Choose a reason for hiding this comment

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

The convention is to use the quotation mark in function names only AFAIK

case Application.get_env(:bundlex, :disable_precompiled_os_deps, false) do
bool when is_boolean(bool) -> bool
[apps: apps] when is_list(apps) -> native.app in apps
end

if native.app in disabled_apps do
if is_precompiled_disabled do
{:error,
"""
is disabled in the application configuration, check the config.exs file.
Copy link
Member

Choose a reason for hiding this comment

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

BTW this error message looks strange. What is disabled in the application configuration?

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 concatenated to a prefix in line 90 :P

Expand All @@ -136,7 +137,7 @@ defmodule Bundlex.Toolchain.Common.Unix.OSDeps do
create_relative_symlink(lib_path, output_path, dep_dir_name)

# TODO: pass the platform via arguments
# $ORIGIN must be escaped soo that it's not treated as an ENV variable
# $ORIGIN must be escaped so that it's not treated as an ENV variable
rpath_root = if Bundlex.platform() == :macosx, do: "@loader_path", else: "\\$ORIGIN"

[
Expand Down Expand Up @@ -183,11 +184,13 @@ defmodule Bundlex.Toolchain.Common.Unix.OSDeps do
|> then(&{:ok, &1})
rescue
e ->
{:error,
"""
couldn't load #{inspect(pkg_configs)} libraries with pkg-config due to:
#{format_exception(e)}
"""}
error = """
couldn't load #{inspect(pkg_configs)} libraries with pkg-config due to:
#{format_exception(e)}
"""

Output.warn(error)
{:error, error}
end
end

Expand Down Expand Up @@ -222,11 +225,13 @@ defmodule Bundlex.Toolchain.Common.Unix.OSDeps do
e ->
File.rm_rf!(package_path)

{:error,
"""
couldn't download and extract the precompiled dependency #{inspect(name)} due to:
#{format_exception(e)}
"""}
error = """
couldn't download and extract the precompiled dependency #{inspect(name)} due to:
#{format_exception(e)}
"""

Output.warn(error)
{:error, error}
end
end
end
Expand Down
2 changes: 1 addition & 1 deletion mix.exs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
defmodule Bundlex.Mixfile do
use Mix.Project

@version "1.4.4"
@version "1.4.5"
@github_url "https://github.com/membraneframework/bundlex"

def project do
Expand Down