Add support for Fuchsia #432

raphlinus · 2016-10-20T16:00:02Z

These patches add support for the Fuchsia operating system. For the
time being, it shares a lot of infrastructure with the Linux target,
because of the use of a musl-based libc.

These patches add support for the Fuchsia operating system. For the time being, it shares a lot of infrastructure with the Linux target, because of the use of a musl-based libc.

rust-highfive · 2016-10-20T16:00:20Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

raphlinus

This is working for me and I think reasonable to commit as-is, but I have a couple questions about whether it's the "right way" to do things.

raphlinus · 2016-10-20T16:04:13Z

src/unix/mod.rs

+    } else if #[cfg(target_os = "fuchsia")] {
+        #[link(name = "c")]
+        #[link(name = "mxio")]
+        #[link(name = "unwind")]


This doesn't seem right to me, but I haven't been able to figure out a better way to get it to work. I tried adding it as #cfg[attr(..., link(...))] at the end of https://github.com/rust-lang/rust/blob/master/src/libunwind/libunwind.rs , but that doesn't seem to show up in the final link line.

I think unwind actually comes in at the libstd layer rather than the libc layer (e.g. those symbols aren't referenced from this library). Other than that though this looks fine to me!

Yeah, I'd like to remove this, but I'm having trouble getting the link to succeed when the link is in libstd rather than here. I'll give that another try...

Ah yeah note that if it's removed here it'll need to be added to src/unwind/build.rs most likely (as that's where the unwinder symbols are defined

Thanks! That's exactly what I was missing. Will respin...

raphlinus · 2016-10-20T16:10:53Z

src/unix/notbsd/linux/mod.rs

@@ -728,6 +728,7 @@ extern {

 cfg_if! {
    if #[cfg(any(target_env = "musl",
+                 target_os = "fuchsia",


So right now this is under linux, because that's where musl lives, and because the diff is really minimal, but I'm not sure that's the best long-term structure. I think it might be better for musl to be a toplevel under notbsd, and have both linux-musl and fuschsia go there, with additional fuchsia #[cfg] items there as needed.

This is fine for now, but if the two start to diverge we can just copy musl over and update fuschia as necessary

posborne · 2016-10-20T16:16:22Z

@raphlinus Would it be possible to add Fuchsia to the set of targets that are tested by CI (see https://github.com/rust-lang/libc/blob/master/ci/README.md)? That is probably the best way to ensure compatibility and support moving forward. For this, we would need to be able to run fuchsia under docker or qemu (guessing qemu would be required in this case).

alexcrichton · 2016-10-20T16:25:57Z

I'd love to add CI as well, but we need to get compiler targets first to ensure we've got a standard library to download. In that sense it's fine by me to punt that to later

raphlinus · 2016-10-20T16:46:58Z

It is certainly possible to run Fuchsia under qemu (see https://fuchsia.googlesource.com/fuchsia/ for instructions how to build the image), and I would like to get CI running. However, things are still stabilizing, so as @alexcrichton suggested I'd like to defer that to later.

The reference to the unwind lib belongs in libstd, not here. Also fix lint error.

alexcrichton · 2016-10-20T17:09:42Z

@bors: r+

bors · 2016-10-20T17:09:43Z

📌 Commit 6d24c4b has been approved by alexcrichton

bors · 2016-10-20T17:10:53Z

⌛ Testing commit 6d24c4b with merge 59b3bbd...

Add support for Fuchsia These patches add support for the Fuchsia operating system. For the time being, it shares a lot of infrastructure with the Linux target, because of the use of a musl-based libc.

tedsta · 2016-10-20T21:40:55Z

src/unix/notbsd/mod.rs

@@ -53,7 +53,9 @@ s! {
        pub ai_protocol: ::c_int,
        pub ai_addrlen: socklen_t,

-        #[cfg(any(target_os = "linux", target_os = "emscripten"))]
+        #[cfg(any(target_os = "linux",
+                  target_os = "emscripten",


Late nitpick - couldn't this be:

#[cfg(any(target_os = "linux", target_os = "android", target_os = "emscripten", target_os = "fuchsia"))] pub ai_addr: *mut ::sockaddr,

rather than having the android entry be separate at line 63?

@tedsta Seems reasonable. This isn't #[repr(C)] so nobody should be caring about the order. I'll patch it and if there are problems, hopefully they'll be caught.

So it turns out that doesn't work, because the structure is serialized, and order matters. Reverted.

Merge two separate config blocks for conditionally including ai_addr in the addrinfo struct for unix/notbsd.

alexcrichton · 2016-10-20T23:10:29Z

@bors: r+

bors · 2016-10-20T23:10:30Z

📌 Commit 7ac91c6 has been approved by alexcrichton

bors · 2016-10-20T23:10:34Z

⌛ Testing commit 7ac91c6 with merge 6077237...

Add support for Fuchsia These patches add support for the Fuchsia operating system. For the time being, it shares a lot of infrastructure with the Linux target, because of the use of a musl-based libc.

alexcrichton · 2016-10-21T00:21:10Z

Looks like CI may be failing on at least Android

This reverts commit 7ac91c6.

alexcrichton · 2016-10-21T01:37:11Z

@bors: r+

bors · 2016-10-21T01:37:13Z

📌 Commit 8c06e14 has been approved by alexcrichton

bors · 2016-10-21T11:46:21Z

⌛ Testing commit 8c06e14 with merge 7880fbc...

Add support for Fuchsia These patches add support for the Fuchsia operating system. For the time being, it shares a lot of infrastructure with the Linux target, because of the use of a musl-based libc.

…target_feature (rust-lang#432) * fix build after stabilization of cfg_target_feature and target_feature * fix doc tests * fix spurious unused_attributes warning * fix more unused attribute warnings * More unnecessary target features * Remove no longer needed trait imports * Remove fixed upstream workarounds * Fix parsing the #[assert_instr] macro Following upstream proc_macro changes * Fix form and parsing of #[simd_test] * Don't use Cargo features for testing modes Instead use RUSTFLAGS with `--cfg`. This'll help us be compatible with the latest Cargo where a tweak to workspaces and features made the previous invocations we had invalid. * Don't thread RUSTFLAGS through docker * Re-gate on x86 verification Closes rust-lang#411

Add support for Fuchsia

517e86d

These patches add support for the Fuchsia operating system. For the time being, it shares a lot of infrastructure with the Linux target, because of the use of a musl-based libc.

rust-highfive assigned alexcrichton Oct 20, 2016

raphlinus commented Oct 20, 2016

View reviewed changes

Remove unwind from link libs

6d24c4b

The reference to the unwind lib belongs in libstd, not here. Also fix lint error.

tedsta reviewed Oct 20, 2016

View reviewed changes

Simplify configuration of ai_addr

7ac91c6

Merge two separate config blocks for conditionally including ai_addr in the addrinfo struct for unix/notbsd.

raphlinus mentioned this pull request Oct 20, 2016

Add Fuchsia support rust-lang/rust#37313

Merged

Revert "Simplify configuration of ai_addr"

8c06e14

This reverts commit 7ac91c6.

alexcrichton merged commit c95defc into rust-lang:master Oct 21, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Fuchsia #432

Add support for Fuchsia #432

raphlinus commented Oct 20, 2016

rust-highfive commented Oct 20, 2016

raphlinus left a comment

raphlinus Oct 20, 2016

alexcrichton Oct 20, 2016

raphlinus Oct 20, 2016

alexcrichton Oct 20, 2016

raphlinus Oct 20, 2016

raphlinus Oct 20, 2016

alexcrichton Oct 20, 2016

posborne commented Oct 20, 2016

alexcrichton commented Oct 20, 2016

raphlinus commented Oct 20, 2016

alexcrichton commented Oct 20, 2016

bors commented Oct 20, 2016

bors commented Oct 20, 2016

tedsta Oct 20, 2016 •

edited

Loading

raphlinus Oct 20, 2016

raphlinus Oct 21, 2016

tedsta Oct 21, 2016

alexcrichton commented Oct 20, 2016

bors commented Oct 20, 2016

bors commented Oct 20, 2016

alexcrichton commented Oct 21, 2016

alexcrichton commented Oct 21, 2016

bors commented Oct 21, 2016

bors commented Oct 21, 2016

Add support for Fuchsia #432

Add support for Fuchsia #432

Conversation

raphlinus commented Oct 20, 2016

rust-highfive commented Oct 20, 2016

raphlinus left a 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

posborne commented Oct 20, 2016

alexcrichton commented Oct 20, 2016

raphlinus commented Oct 20, 2016

alexcrichton commented Oct 20, 2016

bors commented Oct 20, 2016

bors commented Oct 20, 2016

tedsta Oct 20, 2016 • 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

alexcrichton commented Oct 20, 2016

bors commented Oct 20, 2016

bors commented Oct 20, 2016

alexcrichton commented Oct 21, 2016

alexcrichton commented Oct 21, 2016

bors commented Oct 21, 2016

bors commented Oct 21, 2016

tedsta Oct 20, 2016 •

edited

Loading