Skip to content

Conversation

syrusakbary
Copy link
Member

@syrusakbary syrusakbary commented Jan 7, 2020

Description

I disovered this great patch today: https://github.com/MikaelUrankar/webasm/blob/master/www/wasmer/files/patch-freebsd

And then realized that @MikaelUrankar forked wasmer to add support for FreeBSD.

I'm creating this PR to follow up on that fork and merge the changes upstream.

@MikaelUrankar let us know if everything is good to go with this PR!

Review

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

Copy link
Contributor

@nlewycky nlewycky left a comment

Choose a reason for hiding this comment

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

@syrusakbary, do we want a CI for freebsd to go with this?

}

#[cfg(not(target_os = "macos"))]
// Linking to functions that are not provided by rust libc
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this permanent? If not, I'd really appreciate it if this could include a link to an issue or PR.

Choose a reason for hiding this comment

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

I can try to upstream the fn definition.
There is a PR for the ucontext/mcontext part rust-lang/libc#1630

@@ -0,0 +1,127 @@
# NOTE: Keep this consistent with `fault.rs`.
Copy link
Contributor

@nlewycky nlewycky Jan 8, 2020

Choose a reason for hiding this comment

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

I checked it visually and it looks like it's a copy of the linux one. In your opinion should we just rename it to something like sysv-elf and use the same for both, or do you think there's something here that FreeBSD might change and need a different image loader for?

Choose a reason for hiding this comment

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

Yes it's the same as the linux one. I think we can use the same for Linux/FreeBSD.

Copy link
Contributor

@MarkMcCaskey MarkMcCaskey left a comment

Choose a reason for hiding this comment

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

This looks pretty good to me -- I'm super excited about this PR!

I agree with @nlewycky's comments though.

The stuff in emscripten is lower priority, but if parts of rust libc are missing then I think the right thing to do is to add them to Rust libc or at least file an issue and link to the issue in our code!

@MikaelUrankar
Copy link

Hi,
Sorry for the late reply.
The tests pass on amd64, there are some tests that fail on aarch64, I don't know if it's specific to FreeBSD/aarch64 or if it fails on Linux too.

@syrusakbary
Copy link
Member Author

bors try

bors bot added a commit that referenced this pull request Jan 27, 2020
@MarkMcCaskey
Copy link
Contributor

MarkMcCaskey commented Jan 27, 2020

Looks like the failing lint here should be fixed by merging / rebasing with wasmer master

edit: nevermind actually, there were changes to the emscripten crate in this PR!

@syrusakbary
Copy link
Member Author

aarch64 tests seem to be passing :)

@bors
Copy link
Contributor

bors bot commented Jan 27, 2020

try

Build failed

  • wasmerio.wasmer

@MarkMcCaskey
Copy link
Contributor

bors try

bors bot added a commit that referenced this pull request Jan 28, 2020
@bors
Copy link
Contributor

bors bot commented Jan 28, 2020

try

Build succeeded

@MarkMcCaskey
Copy link
Contributor

bors r+

@bors
Copy link
Contributor

bors bot commented Jan 28, 2020

👎 Rejected by too few approved reviews

Copy link
Contributor

@MarkMcCaskey MarkMcCaskey left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks!!

@MarkMcCaskey
Copy link
Contributor

bors r+

bors bot added a commit that referenced this pull request Jan 28, 2020
1120: Port to FreeBSD r=MarkMcCaskey a=syrusakbary

<!-- 
Prior to submitting a PR, review the CONTRIBUTING.md document for recommendations on how to test:
https://github.com/wasmerio/wasmer/blob/master/CONTRIBUTING.md#pull-requests

-->

# Description

I disovered this great patch today: https://github.com/MikaelUrankar/webasm/blob/master/www/wasmer/files/patch-freebsd

And then realized that @MikaelUrankar forked wasmer to add support for FreeBSD.

I'm creating this PR to follow up on that fork and merge the changes upstream.

@MikaelUrankar let us know if everything is good to go with this PR!

<!-- 
Provide details regarding the change including motivation,
links to related issues, and the context of the PR.
-->

# Review

- [ ] Add a short description of the the change to the CHANGELOG.md file


Co-authored-by: MikaelUrankar <[email protected]>
@bors
Copy link
Contributor

bors bot commented Jan 28, 2020

Build succeeded

@bors bors bot merged commit fcbdada into wasmerio:master Jan 28, 2020
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.

4 participants