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

Add fuchsia support #1285

Merged
merged 2 commits into from Dec 19, 2020
Merged

Add fuchsia support #1285

merged 2 commits into from Dec 19, 2020

Conversation

amanda-tait
Copy link
Contributor

This change adds support for the Fuchsia operating system to the nix
crate. Fuchsia developers have a use case for nix, particularly its safe
interfaces to the recvmsg(2) and sendmsg(2). Adding support requires:

  • incrementing the libc dependency to 0.2.74
  • conditionally not compiling nix functionality which depends on libc functionality
    that does not exist for Fuchsia

@amanda-tait
Copy link
Contributor Author

@asomers @kamalmarhubi @posborne

Friendly bump for code review. Thank you for taking a look.

@posborne
Copy link
Member

@amanda-tait The change looks reasonable to me, is there any reasonable path for adding fuchsia support to CI. Without that, I fear it is pretty easy for other contributors to easily introduce regressions. I do not see CI support (or missed it) within libc either.

@asomers
Copy link
Member

asomers commented Aug 22, 2020

I see that rustup has a fuchsia target. If nothing else, we can add a CI target that cross-compiles to fuchsia, but doesn't run tests.

@tamird
Copy link
Contributor

tamird commented Sep 30, 2020

@posborne CI support is something we'd like to have, but Fuchsia does not yet provide tools for external CI.

We're OK with taking on the burden of fixing regressions.

@asomers
Copy link
Member

asomers commented Oct 3, 2020

The change looks good, but please add a CHANGELOG entry.

@tamird
Copy link
Contributor

tamird commented Nov 5, 2020

@asomers added a CHANGELOG entry and resolved the merge conflict.

@tamird
Copy link
Contributor

tamird commented Nov 5, 2020

@asomers I think cross compiling to Fuchsia would be great, do you have a suggestion on how to do that?

EDIT: added

@tamird tamird force-pushed the fuchsia branch 7 times, most recently from 7303358 to cc46340 Compare November 11, 2020 19:54
@tamird
Copy link
Contributor

tamird commented Nov 12, 2020

@asomers I added a pair of commits to address upstream deprecations in libc, and this is now green and includes cross compilation to Fuchsia. Can you take a look?

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

Your PR now conflicts with #1329. But that PR requires your musl fix in order to build. Could you please split this PR into two? One for the musl fix, one for Fuchsia, and discard the af_alg_iv parts. Also, add Fuchsia to Tier 3 in the README.

.travis.yml Outdated
- curl --proto '=https' --tlsv1.2 -sSf --output rustup.sh https://sh.rustup.rs
- sh rustup.sh -y --profile=minimal --default-toolchain 1.36.0 --target x86_64-fuchsia
- . $HOME/.cargo/env
- cargo build --all-targets
Copy link
Member

Choose a reason for hiding this comment

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

You need to add --target x86_64-fuchsia to these two lines. (you probably copied this from the Redox section, which makes the same mistake; I'll fix that separately).

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is incorrect; the rustup invocation only installs the fuchsia target, so this does the right thing. The same is true for redox.

Copy link
Member

Choose a reason for hiding this comment

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

It is incorrect. I just checked in a fresh environment. The rustup invocation installs both the native toolchain and the one that you request with --target. So you need to do one of tho things:

  • Use --default-host instead of --target, or
  • Add --target x86_64-fuchsia to the cargo build line, as I suggested.

@tamird
Copy link
Contributor

tamird commented Nov 15, 2020

I can make the changes you requested, but I think the approach taken in #1329 is incorrect. As I mentioned in that PR, the deprecation is informational, and the approach taken there is far more brittle without any upside.

@tamird
Copy link
Contributor

tamird commented Nov 15, 2020

Added Fuchsia to Tier 3 in the README.

@tamird
Copy link
Contributor

tamird commented Nov 16, 2020

@asomers rebased now that #1332 is in.

@tamird
Copy link
Contributor

tamird commented Nov 16, 2020

CI is passing.

.travis.yml Outdated
- curl --proto '=https' --tlsv1.2 -sSf --output rustup.sh https://sh.rustup.rs
- sh rustup.sh -y --profile=minimal --default-toolchain 1.36.0 --target x86_64-fuchsia
- . $HOME/.cargo/env
- cargo build --all-targets
Copy link
Member

Choose a reason for hiding this comment

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

It is incorrect. I just checked in a fresh environment. The rustup invocation installs both the native toolchain and the one that you request with --target. So you need to do one of tho things:

  • Use --default-host instead of --target, or
  • Add --target x86_64-fuchsia to the cargo build line, as I suggested.

@tamird tamird force-pushed the fuchsia branch 2 times, most recently from 0f5a57b to bef48a2 Compare November 29, 2020 01:29
@tamird
Copy link
Contributor

tamird commented Nov 29, 2020

@asomers I can't seem to reply to your inline comment, but I've made the changes you requested and added Fuchsia to the list added in #1337.

@tamird
Copy link
Contributor

tamird commented Dec 15, 2020

Done.

@tamird
Copy link
Contributor

tamird commented Dec 15, 2020

Another flake.

---- test_fcntl::linux_android::test_vmsplice stdout ----
thread 'test_fcntl::linux_android::test_vmsplice' panicked at 'assertion failed: `(left == right)`
  left: `6`,
 right: `3`', test/test_fcntl.rs:198:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

@asomers
Copy link
Member

asomers commented Dec 16, 2020

Another flake.

---- test_fcntl::linux_android::test_vmsplice stdout ----
thread 'test_fcntl::linux_android::test_vmsplice' panicked at 'assertion failed: `(left == right)`
  left: `6`,
 right: `3`', test/test_fcntl.rs:198:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

Sigh. This looks like a kernel bug. Or possibly a documentation bug. Is vmsplice allowed to write fewer bytes than were requested? The docs explicitly say that syscalls like write and sendfile may do that, but the man page for vmsplice is silent on the matter. In any case, it's certainly unrelated to this PR. For now, I'll try to rerun the check.

@tamird
Copy link
Contributor

tamird commented Dec 16, 2020

Thanks, looks like rerunning it worked. Is this good to land?

@asomers
Copy link
Member

asomers commented Dec 16, 2020

No, not yet. You still have that duped entry in the changelog.

@tamird
Copy link
Contributor

tamird commented Dec 16, 2020

Ah, sorry. It ended up in the second commit somehow. Done.

@tamird
Copy link
Contributor

tamird commented Dec 16, 2020

This is green again and I've made all the changes requested. Please have another look.

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

bors r+

bors bot added a commit that referenced this pull request Dec 16, 2020
1285: Add fuchsia support r=asomers a=amanda-tait

This change adds support for the Fuchsia operating system to the nix
crate. Fuchsia developers have a use case for nix, particularly its safe
interfaces to the recvmsg(2) and sendmsg(2). Adding support requires:
* incrementing the libc dependency to 0.2.74
* conditionally not compiling nix functionality which depends on libc functionality
  that does not exist for Fuchsia

Co-authored-by: Tamir Duberstein <tamird@google.com>
Co-authored-by: Amanda Tait <atait@google.com>
@tamird
Copy link
Contributor

tamird commented Dec 18, 2020

Is something wedged? Looks like this has been "waiting in queue" for 2 days (and I can't see the bors details page).

@asomers
Copy link
Member

asomers commented Dec 18, 2020

Cirrus is fine, but Bors is crashing.
bors-ng/bors-ng#1106
Hopefully fixed by #1365 .

@tamird
Copy link
Contributor

tamird commented Dec 18, 2020

Looks like that worked. Rebased.

@tamird
Copy link
Contributor

tamird commented Dec 18, 2020

@asomers looks like a network flake this time

error: could not download file from 'https://static.rust-lang.org/dist/channel-rust-1.40.0.toml.sha256' to '/tmp/home/.rustup/tmp/4t0mcegb2twtxqn5_file'

Would you mind retrying it and submitting this once more?

Also, I'm not able to edit the PR description, but you might be able to -- could you edit it to reflect the second commit's message?

@asomers
Copy link
Member

asomers commented Dec 19, 2020

Also, I'm not able to edit the PR description, but you might be able to -- could you edit it to reflect the second commit's message?

What's wrong with the PR description?

@tamird
Copy link
Contributor

tamird commented Dec 19, 2020

The libc version is not modified.

tamird and others added 2 commits December 19, 2020 14:17
Allow nix to compile on Fuchsia by conditionally avoiding libc
functionality that does not exist for Fuchsia.
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

bors r+

@bors bors bot merged commit d07b7f8 into nix-rust:master Dec 19, 2020
@tamird tamird deleted the fuchsia branch December 19, 2020 22:56
asomers added a commit that referenced this pull request Aug 18, 2021
* I never removed 32-bit OSX and iOS targets that were removed from
  Cirrus in PR #1492 .
* I never added Fuchsia (PR #1285) .
* I never added illumos (PR #1394) .
* never added Linux x32 (PR #1366) .
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.

None yet

5 participants