Skip to content

Add Redox support for most of the modules #1098

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

Merged
merged 13 commits into from
May 20, 2020

Conversation

AdminXVII
Copy link
Contributor

Some things are not implemented yet in redox, so a lot of annotations were added to remove functions when compiling for redox. Those functions will hopefully be added in time, but for now it's better to have partial support than none.

Blocked by rust-lang/libc#1438

@asomers
Copy link
Member

asomers commented Jul 12, 2019

Interesting! But I have two big questions:

  1. What's the best way to get Redox running in a VM? The last time I tried I couldn't get it to work.
  2. How would you test on Redox in CI?

@AdminXVII
Copy link
Contributor Author

There is redoxer that does exactly this. You basically pull the redoxos/redoxer docker image and then run redoxer test

@asomers asomers added this to the 0.16.0 milestone Jul 12, 2019
@AdminXVII
Copy link
Contributor Author

@AdminXVII AdminXVII force-pushed the add-redox-support branch from 5e6db09 to 6fd6456 Compare July 22, 2019 17:46
@AdminXVII
Copy link
Contributor Author

The libc request was merged.

@AdminXVII AdminXVII force-pushed the add-redox-support branch 3 times, most recently from fcf97ca to 94a4d2b Compare September 12, 2019 16:23
@AdminXVII
Copy link
Contributor Author

Btw, should I include the tests for redox in this PR or should I make another one specifically for it?

@asomers
Copy link
Member

asomers commented Sep 16, 2019

Best if you can add them to this PR.

@AdminXVII AdminXVII force-pushed the add-redox-support branch 12 times, most recently from b8498fb to 2a560e4 Compare September 17, 2019 15:04
@AdminXVII
Copy link
Contributor Author

@asomers the CI succeeded and I only added a new test. Is the problem resolved?

@asomers
Copy link
Member

asomers commented Sep 17, 2019

Amazing! You fixed it! ;). I guess Travis undid whatever they did on September 6. I see a lot of warnings in the Redox test build, though. Could you please silence all of the warnings?

@AdminXVII
Copy link
Contributor Author

I was using the warnings to remember what needs to be included. I'd have silenced them when you would have been ready to merge. Is that ok with you?

@asomers
Copy link
Member

asomers commented Sep 17, 2019

You mean you want to silence them after I've reviewed the rest of the PR? That's ok.

@AdminXVII
Copy link
Contributor Author

There is currently ongoing work to improve what redox can do, so as this goes I remove cfgs to add more things for Redox. I used the comment to see what was needed to add. That said I think it's better if I remove and simply grep, so discard my previous comment.

@AdminXVII
Copy link
Contributor Author

1- Qemu is only necessary to run it. To compile, you'd simply do gmake all.
2- Not that I'm aware of.

@@ -153,21 +161,21 @@ libc_bitflags!(

pub fn open<P: ?Sized + NixPath>(path: &P, oflag: OFlag, mode: Mode) -> Result<RawFd> {
let fd = path.with_nix_path(|cstr| {
let modebits = c_uint::from(mode.bits());
unsafe { libc::open(cstr.as_ptr(), oflag.bits(), modebits) }
unsafe { libc::open(cstr.as_ptr(), oflag.bits(), mode.bits() as c_uint) }
Copy link
Member

Choose a reason for hiding this comment

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

Why did you make this change? Using from is usually preferable to as.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's because mode_t is a signed int on redox, so in terms of integer, because it can be lossy, the From<c_uint> trait is not implemented for c_int but as c_uint works fine.

pub fn openat<P: ?Sized + NixPath>(dirfd: RawFd, path: &P, oflag: OFlag, mode: Mode) -> Result<RawFd> {
let fd = path.with_nix_path(|cstr| {
let modebits = c_uint::from(mode.bits());
unsafe { libc::openat(dirfd, cstr.as_ptr(), oflag.bits(), modebits) }
unsafe { libc::openat(dirfd, cstr.as_ptr(), oflag.bits(), mode.bits() as c_uint) }
Copy link
Member

Choose a reason for hiding this comment

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

Ditto from/as.

@AdminXVII AdminXVII closed this May 7, 2020
@AdminXVII AdminXVII reopened this May 7, 2020
@AdminXVII
Copy link
Contributor Author

The as cast is needed because mode_t is signed on redox while unsigned on open. From<c_uint> is not implemented for c_int, but it's not a problem since this will leave the bits in place, which is the expected behaviour.

@AdminXVII AdminXVII force-pushed the add-redox-support branch from 3320483 to ef07a8e Compare May 14, 2020 06:09
AdminXVII and others added 12 commits May 17, 2020 21:05
Some things are not implemented yet in redox, so a lot of annotations
were added to remove functions when compiling for redox. Those functions
will hopefully be added in time, but for now it's better to have partial
support than none.

Blocked by rust-lang/libc#1438
FIFOs are not supported (yet?) by RedoxFS, so disable it
 - Make sure all tests pass the CI
 - Redox does not (yet) have passwd functions, so remove it
@AdminXVII AdminXVII force-pushed the add-redox-support branch from ef07a8e to bedbb8d Compare May 18, 2020 01:06
@AdminXVII
Copy link
Contributor Author

Rebased

@asomers
Copy link
Member

asomers commented May 20, 2020

Ok, could you please do one last thing? Just add Redox to the list of supported platforms in the README.md. Put it in the "Tier 3" section.

@AdminXVII
Copy link
Contributor Author

Sure it's done!

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.

Let's do it!

bors r+

@AdminXVII
Copy link
Contributor Author

Yes! Thanks a lot for your time, it's greatly appreciated!

@bors
Copy link
Contributor

bors bot commented May 20, 2020

Build succeeded:

@bors bors bot merged commit d950c48 into nix-rust:master May 20, 2020
asomers added a commit to asomers/nix that referenced this pull request May 31, 2020
This test cannot be compiled under Redox.  PR nix-rust#1098 attempted to disable
it for Redox, but actually disabled it everywhere.  AFAICT, Cargo has no
syntax to conditionally enable a target, except based on features.
Instead, use conditional compilation within the test.
asomers added a commit that referenced this pull request Jun 1, 2020
This test cannot be compiled under Redox.  PR #1098 attempted to disable
it for Redox, but actually disabled it everywhere.  AFAICT, Cargo has no
syntax to conditionally enable a target, except based on features.
Instead, use conditional compilation within the test.
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