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

Expand cross compilation #124

Merged
merged 22 commits into from
Mar 18, 2024
Merged

Expand cross compilation #124

merged 22 commits into from
Mar 18, 2024

Conversation

Noarkhh
Copy link
Contributor

@Noarkhh Noarkhh commented Feb 23, 2024

closes membraneframework/membrane_core#721

Up until now there were two public functions that gave information about platform, Bundlex.get_target/0 and Bundlex.platform/0. Their difference was that get_target/0 gave more specific infomation, but later on, when adding crosscompilation support, only platform/0 detected the crosscompilation.

This PR changes behavior of Bundlex.get_target/0, so that when crosscompiling the target map will have it's fields set by environment variables used by Nerves.

This PR also allows for crosscompilation outside of Nerves. The only thing nerves does is set env variables to correct flags and paths, which can be also done by the user that wants to use their custom toolchain.

lib/bundlex.ex Outdated Show resolved Hide resolved
lib/bundlex.ex Show resolved Hide resolved
@@ -47,7 +81,7 @@ defmodule Bundlex do
@doc """
Returns family of the platform obtained with `platform/0`.
"""
@spec family() :: :unix | :windows
@spec family() :: :unix | :windows | :custom
Copy link
Member

Choose a reason for hiding this comment

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

Since we deprecate this function anyway, maybe let's keep it unchanged for better compatibility?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we deprecate platform, not family

README.md Outdated
@@ -172,6 +174,20 @@ As in the case of NIFs, CNodes compiled with Bundlex can be used like any other
Similarly to CNodes Bundlex provides `Bundlex.Port` module for a little easier interacting with Ports.
Please refer to the module's documentation to see how to use it.

### Cross-compilation

With proper setup Bundlex can support cross-compilation. When using Nerves the it should work out of the box.
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
With proper setup Bundlex can support cross-compilation. When using Nerves the it should work out of the box.
With proper setup, Bundlex can support cross-compilation. When using Nerves it should work out of the box.

README.md Outdated

With proper setup Bundlex can support cross-compilation. When using Nerves the it should work out of the box.

Not relying in Nerves and using your own toolchain is also possible, although it wasn't tested. In this scenario the following environment variables need to be set, since they're not handled by Nerves:
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
Not relying in Nerves and using your own toolchain is also possible, although it wasn't tested. In this scenario the following environment variables need to be set, since they're not handled by Nerves:
Not relying on Nerves and using your own toolchain is also possible, although it wasn't tested. In this scenario, the following environment variables need to be set:

lib/bundlex.ex Outdated
Comment on lines 21 to 25
%{
architecture: String.t() | :unknown,
vendor: String.t() | :unknown,
os: String.t() | :unknown,
abi: String.t() | :unknown
Copy link
Member

Choose a reason for hiding this comment

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

I'd either use nil or "unknown"

lib/mix/tasks/compile.bundlex.ex Show resolved Hide resolved
@Noarkhh Noarkhh requested a review from mat-hek February 26, 2024 12:38
:nerves

:error ->
Output.warn(
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
Output.warn(
Output.info(

lib/bundlex.ex Outdated Show resolved Hide resolved
@Noarkhh Noarkhh requested a review from mat-hek February 28, 2024 14:42
orbs:
elixir: membraneframework/elixir@1
elixir: membraneframework/elixir@dev:94bbc77eef4293223822a2f7f31b136ac0833b1f
Copy link
Member

@FelonEkonom FelonEkonom Feb 29, 2024

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm experimenting with using non-membrane images where they're not required membraneframework/membrane_core#701

Copy link
Member

Choose a reason for hiding this comment

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

Remember to fix it before merging

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
lib/bundlex.ex Outdated Show resolved Hide resolved
lib/bundlex.ex Show resolved Hide resolved
lib/bundlex/platform.ex Show resolved Hide resolved
@@ -147,7 +147,7 @@ defmodule Bundlex.Toolchain.Common.Unix.OSDeps do

# TODO: pass the platform via arguments
# $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"
rpath_root = if Platform.get_target!() == :macosx, do: "@loader_path", else: "\\$ORIGIN"
Copy link
Member

Choose a reason for hiding this comment

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

What is the difference between Platform.get_target!() and Bundlex.get_target!()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now deprecated Bundlex.get_platform/0 invoked Platform.get_target!/0, it gives a bit different information than Bundlex.get_target/0, we probably want to get rid of it eventually and rely just on the Bundlex.get_target/0

Copy link
Member

Choose a reason for hiding this comment

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

So why won't you use Bundlex.get_target/0 instead of Platform.get_target!/0?

@Noarkhh Noarkhh force-pushed the expand-cross-compilation branch 6 times, most recently from 07c6eb3 to c73d0ec Compare March 4, 2024 14:14
@Noarkhh Noarkhh force-pushed the expand-cross-compilation branch 2 times, most recently from d02d01e to 2194cb8 Compare March 4, 2024 14:56
Copy link
Member

@FelonEkonom FelonEkonom left a comment

Choose a reason for hiding this comment

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

Let me know, when it will be ready for review

@Noarkhh Noarkhh force-pushed the expand-cross-compilation branch 2 times, most recently from 3cb3c68 to 70281f4 Compare March 4, 2024 16:41
orbs:
elixir: membraneframework/elixir@1
elixir: membraneframework/elixir@dev:527ac9efd5554630ed90dbe24cd35b45dedcca9a
Copy link
Member

Choose a reason for hiding this comment

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

seems like a temporary value

@Noarkhh Noarkhh force-pushed the expand-cross-compilation branch 2 times, most recently from 30c17f8 to ac0d94a Compare March 18, 2024 13:21
@Noarkhh Noarkhh merged commit a03b048 into master Mar 18, 2024
3 checks passed
@Noarkhh Noarkhh deleted the expand-cross-compilation branch March 18, 2024 13:58
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.

[Bundlex] Reconsider public API
3 participants