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

iOS Support - via .dylib & Headless Runtime #2516

Closed

Conversation

NathHorrigan
Copy link
Contributor

Description

This PR makes some fairly simple changes to feature flags so that forks originally intended *-apple-darwin now work for both aarch64-apple-ios/x86_64-apple-ios.

Review

  • Add a short description of the change to the CHANGELOG.md file

@@ -23,7 +23,6 @@ use wasmer_engine_universal::Universal;
///
/// This is a Wasmer-specific type with Wasmer-specific functions for
/// manipulating it.
#[cfg(feature = "compiler")]
Copy link
Member

Choose a reason for hiding this comment

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

Undelete this :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doing so causes this:

error[E0432]: unresolved import `super::super::engine::wasmer_compiler_t`
 --> lib/c-api/src/wasm_c_api/unstable/engine.rs:4:43
  |
4 | use super::super::engine::{wasm_config_t, wasmer_compiler_t, wasmer_engine_t};
  |                                           ^^^^^^^^^^^^^^^^^ no `wasmer_compiler_t` in `wasm_c_api::engine`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which is used by this method:

pub extern "C" fn wasmer_is_compiler_available(compiler: wasmer_compiler_t) -> bool {
    match compiler {
        wasmer_compiler_t::CRANELIFT if cfg!(feature = "cranelift") => true,
        wasmer_compiler_t::LLVM if cfg!(feature = "llvm") => true,
        wasmer_compiler_t::SINGLEPASS if cfg!(feature = "singlepass") => true,
        _ => false,
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Please, keep #[cfg(feature = "compiler")] here. But add:

use super::super::engine:{wasm_config_t, wasmer_engine_t};
#[cfg(feature = "compiler")]
use super::super::engine::wasmer_compiler_t;

in lib/c-api/src/wasm_c_api/unstable/engine.rs. It's a bug here.

Thank you very much!

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 added those compiler conditions, I had to add some others to fix more build errors though?

lib/api/Cargo.toml Outdated Show resolved Hide resolved
lib/c-api/Cargo.toml Outdated Show resolved Hide resolved
@NathHorrigan NathHorrigan force-pushed the feature/ios-headless-support branch from 00e87ed to 35fd710 Compare August 11, 2021 20:58
@syrusakbary
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Aug 12, 2021
@bors
Copy link
Contributor

bors bot commented Aug 12, 2021

try

Build failed:

@NathHorrigan
Copy link
Contributor Author

NathHorrigan commented Aug 12, 2021

bors try

Edit: Eh... nice try

@bors
Copy link
Contributor

bors bot commented Aug 12, 2021

🔒 Permission denied

Existing reviewers: click here to make NathHorrigan a reviewer

Copy link
Contributor

@Hywan Hywan left a comment

Choose a reason for hiding this comment

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

Thank you very much for your work! I believe we are on a good track. I hope my review provides relevant inputs :-).

Makefile Show resolved Hide resolved
examples/engine_headless_ios.rs Outdated Show resolved Hide resolved
examples/engine_headless_ios.rs Outdated Show resolved Hide resolved
examples/engine_headless_ios.rs Outdated Show resolved Hide resolved
examples/engine_headless_ios.rs Outdated Show resolved Hide resolved
@@ -23,7 +23,6 @@ use wasmer_engine_universal::Universal;
///
/// This is a Wasmer-specific type with Wasmer-specific functions for
/// manipulating it.
#[cfg(feature = "compiler")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, keep #[cfg(feature = "compiler")] here. But add:

use super::super::engine:{wasm_config_t, wasmer_engine_t};
#[cfg(feature = "compiler")]
use super::super::engine::wasmer_compiler_t;

in lib/c-api/src/wasm_c_api/unstable/engine.rs. It's a bug here.

Thank you very much!

lib/c-api/src/wasm_c_api/unstable/parser/operator.rs Outdated Show resolved Hide resolved
lib/engine-dylib/src/artifact.rs Outdated Show resolved Hide resolved
lib/engine-dylib/src/artifact.rs Outdated Show resolved Hide resolved
@Hywan Hywan self-assigned this Aug 12, 2021
@Hywan Hywan added 🍎 platform-darwin This issue happens on macOS 🎉 enhancement New feature! 📦 lib-engine-dylib About wasmer-engine-dylib labels Aug 12, 2021
@Hywan Hywan linked an issue Aug 12, 2021 that may be closed by this pull request
@NathHorrigan
Copy link
Contributor Author

That should be ready for you now @syrusakbary !

@syrusakbary
Copy link
Member

bors try

@NathHorrigan
Copy link
Contributor Author

I also updated the Xcode build scripts so you shouldn't have any issues on your M1 machine :)

@syrusakbary
Copy link
Member

bors try

@bors
Copy link
Contributor

bors bot commented Aug 18, 2021

try

Already running a review

@syrusakbary
Copy link
Member

bors try-

@syrusakbary
Copy link
Member

bors try

@syrusakbary
Copy link
Member

bors try-

@syrusakbary
Copy link
Member

bors try

@syrusakbary
Copy link
Member

Bors is weirdly failing:

{{:badmatch,   {:error, :merge_branch, 403,    "{\"message\":\"Resource not accessible by integration\",\"documentation_url\":\"https://docs.github.com/rest/reference/repos#merge-a-branch\"}",    "8D04:1534:5623A3:D8B0BD:611ED99C"}},  [    {BorsNG.GitHub, :merge_branch!, 2,     [file: 'lib/github/github.ex', line: 185]},    {BorsNG.Worker.Attemptor, :start_attempt, 2,     [file: 'lib/worker/attemptor.ex', line: 204]},    {BorsNG.Worker.Attemptor, :handle_cast, 2,     [file: 'lib/worker/attemptor.ex', line: 61]},    {:gen_server, :try_dispatch, 4,     [file: 'gen_server.erl', line: 695]},    {:gen_server, :handle_msg, 6,     [file: 'gen_server.erl', line: 771]},    {:proc_lib, :init_p_do_apply, 3,     [file: 'proc_lib.erl', line: 226]}  ]}
--

@NathHorrigan
Copy link
Contributor Author

That's very strange, according to Google that is an error caused when the userr calling Bors doesn't have permission. Do you wanna try again?

@NathHorrigan
Copy link
Contributor Author

@syrusakbary ^^

@syrusakbary
Copy link
Member

bors try

@bors
Copy link
Contributor

bors bot commented Aug 20, 2021

try

Already running a review

@syrusakbary
Copy link
Member

bors try-

@syrusakbary
Copy link
Member

bors try

@syrusakbary
Copy link
Member

I think is because github actions file got changed. I'll close this PR and reopen as a new one from Wasmer (also I'll add you as contributor so you can directly push to the feature/ios-headless-support branch in this repo (without needing a fork)

@syrusakbary syrusakbary mentioned this pull request Aug 20, 2021
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍎 platform-darwin This issue happens on macOS 🎉 enhancement New feature! 📦 lib-engine-dylib About wasmer-engine-dylib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

iOS: wasmer attempts to allocate too much memory
3 participants