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.

Sign up for GitHub

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

Jump to bottom

illumos and Solaris support #1394

Merged
merged 1 commit into from
Mar 21, 2021
Merged

illumos and Solaris support #1394

merged 1 commit into from
Mar 21, 2021

Conversation

jasonbking
Copy link
Contributor

Adds support for the illumos target as well as improvements in the existing Solaris support.

Co-authored-by: Dominik Hassler [email protected]
Co-authored-by: Joshua M. Clulow [email protected]

@jasonbking
Copy link
Contributor Author

There are three tests that currently fail on illumos:

  • sys::test_termios::test_local_flags
  • test_pty::test_read_ptty_pair
  • test_unistd::test_ttyname

However those tests share a mutex with other tests. Their failures cause some additional tests to fail due to a poisoned mutex error:

  • sys::test_termios::test_output_flags
  • sys::test_termios::test_tcgetattr_pty
  • test_pty::test_forkpty
  • test_pty::test_open_ptty_pair
  • test_pty::test_openpty
  • test_pty::test_openpty_with_termios
  • test_pty::test_write_ptty_pair

When the above tests are run individually, they pass.

I'm still researching the three actual failures, though the remaining functionality works well enough for stuff using the nix crate to build successfully with this commit on illumos, so it seemed useful even without those resolved.

asomers
asomers requested changes Mar 4, 2021
View reviewed changes
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.

In addition to the inline comments, could you also add Illumos to CI ? You can put it in the cross-build section, along with NetBSD and Fuchsia, for OSes that don't have their tests run in CI.

src/dir.rs Outdated
pub fn file_type(&self) -> Option<Type> {
#[cfg(not(target_os = "illumos"))]
Copy link
Member

Choose a reason for hiding this comment

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

Do you really need this line, given that file_type isn't compiled on Illumos?

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 understand why the whole of file_type() was made conditional for illumos. In my original patch here it was not, because it seemed like it would then require less changes for consuming code -- we just return None in all cases and then the consumer knows (as per the documentation) to do their own stat(2). @jasonbking ?

Copy link
Contributor

@jclulow jclulow Mar 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Oh my apologies, I missed the whole separate file_type() below. This must be the result of merging our two porting efforts. Sorry about that!

src/dir.rs Outdated
@@ -236,5 +240,14 @@ impl Entry {
libc::DT_SOCK => Some(Type::Socket),
/* libc::DT_UNKNOWN | */ _ => None,
}

// illumos systems do not have the d_type member at all:
#[cfg(target_os = "illumos")]
Copy link
Member

Choose a reason for hiding this comment

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

Ditto here. Can you remove this part?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this was a merge artifact, I'll fix it up.

src/errno.rs
@@ -25,6 +25,10 @@ cfg_if! {
unsafe fn errno_location() -> *mut c_int {
libc::__errno_location()
}
} else if #[cfg(any(target_os = "illumos", target_os = "solaris"))] {
Copy link
Member

Choose a reason for hiding this comment

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

Can this be combined with the clause on line 9?

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 hard to see in the browser, but on illumos and Solaris, it's three underscores, but only two on android/netbsd/openbsd, so I don't believe it can (unless I'm misunderstanding how you wanted to combine them).

Copy link
Member

Choose a reason for hiding this comment

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

You're right; that's awfully hard to see.

src/errno.rs Outdated Show resolved Hide resolved
src/sys/ioctl/illumos.rs Outdated
@@ -0,0 +1,92 @@
/// The datatype used for the ioctl number
Copy link
Member

Choose a reason for hiding this comment

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

If you combine this code with bsd.rs, how much #[cfg] stuff would you have to add? The files look pretty similar.

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 think the only difference would be for ioctl_num_type (c_int vs c_long). I could do that, though is there a preference here on how (one file w/ a name suggesting both, re-export the shared bits, etc.)?

Copy link
Member

Choose a reason for hiding this comment

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

The file name isn't part of the public API, so it doesn't matter very much. I would probably call it "bsd" just as the path of least resistance.

@asomers
Copy link
Member

asomers commented Mar 5, 2021

If you rebase on master that will fix the Android build failures. For Illumos, it looks like you need to choose a more recent toolchain.

@jasonbking
Copy link
Contributor Author

jasonbking commented Mar 5, 2021 •
edited
Loading

Do you prefer a specific (or rather explicit) version, or is 'stable' good enough?

@asomers
Copy link
Member

asomers commented Mar 6, 2021

Ok, I think this looks good now. Would you mind squashing your commits?

@jasonbking
Copy link
Contributor Author

Should be good now.

@jasonbking jasonbking requested a review from asomers March 10, 2021 02:14
@davepacheco davepacheco mentioned this pull request Mar 10, 2021
Merged
asomers
asomers approved these changes Mar 21, 2021
View reviewed changes
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 Mar 21, 2021
@bors @jasonbking
Merge #1394
dfa7849 
1394: illumos and Solaris support r=asomers a=jasonbking

Adds support for the illumos target as well as improvements in the existing Solaris support.

Co-authored-by: Dominik Hassler <[email protected]>
Co-authored-by: Joshua M. Clulow <[email protected]>

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

bors bot commented Mar 21, 2021

Build failed:

@jasonbking
Copy link
Contributor Author

I'm a bit stumped at the build error -- nothing in the PR should have touched (directly or indirectly) the Linux VSOCK support (and checking again, I can't see anything that appears to). Have I possibly overlooked something?

@jasonbking
Copy link
Contributor Author

I also looked at the errno changes and can't see anything that should have impacted Linux.

@asomers
Copy link
Member

asomers commented Mar 21, 2021

You'll have to rebase after #1404 .

@jasonbking @hadfl @jclulow
illumos and Solaris support
d444f1b 
Co-authored-by: Dominik Hassler <[email protected]>
Co-authored-by: Joshua M. Clulow <[email protected]>
@jasonbking
Copy link
Contributor Author

Phew :)

asomers
asomers approved these changes Mar 21, 2021
View reviewed changes
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
Copy link
Contributor

bors bot commented Mar 21, 2021

Build succeeded:

@bors bors bot merged commit fde9ebc into nix-rust:master Mar 21, 2021
@jasonbking jasonbking deleted the illumos-squash branch March 21, 2021 23:40
asomers added a commit that referenced this pull request Aug 18, 2021
@asomers
[skip ci] Fix bors configuration
0163528 
* 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) .
@gco gco mentioned this pull request Jan 29, 2023
Closed
@travispaul travispaul mentioned this pull request Dec 22, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jclulow jclulow jclulow left review comments

@asomers asomers asomers approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jasonbking @asomers @jclulow