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

haiku: add platform support. #772

Closed
wants to merge 1 commit into from

Conversation

jessicah
Copy link

No description provided.

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.

Moving constants into libc is important. I haven't finished reviewing this PR yet, but I'll try to get to it tonight. Stylistically, I have concerns about the use of #[cfg(...)] vs #[cfg(not(...))]. We should consistently use either the positive form or the negative form in each block of code, and not mix them.

@@ -1910,6 +1925,172 @@ mod consts {
}
}

#[cfg(target_os = "haiku")]
Copy link
Member

Choose a reason for hiding this comment

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

Nix does not define constants anymore. Instead, all of these should be defined in the libc crate, and nix should reference libc.

Copy link
Author

Choose a reason for hiding this comment

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

You mean how the linux version is done? I had wondered why it was different to the other platforms.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah. It looks like Somebody fixed the Linux definitions in change 3354ffe , but not other OS's.

@@ -47,6 +47,12 @@ unsafe fn errno_location() -> *mut c_int {
__errno()
}

#[cfg(target_os = "haiku")]
unsafe fn errno_location() -> *mut c_int {
extern { fn _errnop() -> *mut c_int; }
Copy link
Contributor

Choose a reason for hiding this comment

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

This should also be declared in libc instead of here.

@jessicah
Copy link
Author

Restricting to just the 'positive' would create unnecessarily complex #[cfg(...)] attributes, wouldn't it?
Having to list every supported OS instead seems a bit cumbersome. If I go this route, I would prefer to list the targets one per line throughout. Would that be acceptable? Scrolling horizontally is a pain ;-)

@@ -28,6 +28,7 @@ libc_bitflags!(
O_ALT_IO;
/// Open the file in append-only mode.
O_APPEND;
#[cfg(not(target_os = "haiku"))]
Copy link
Contributor

Choose a reason for hiding this comment

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

These should actually go underneath the comment.

@@ -92,7 +92,7 @@ mod os {
}
}

#[cfg(any(target_os = "macos", target_os = "freebsd", target_os = "dragonfly", target_os = "ios", target_os = "openbsd", target_os = "netbsd"))]
#[cfg(any(target_os = "macos", target_os = "freebsd", target_os = "dragonfly", target_os = "ios", target_os = "openbsd", target_os = "netbsd", target_os = "haiku"))]
Copy link
Contributor

Choose a reason for hiding this comment

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

Please line wrap these.

@Susurrus
Copy link
Contributor

@jessicah You have any time to get around to resolving this? We'd love to have Haiku support!

@Susurrus
Copy link
Contributor

@jessicah Wanted to ping you again on this.

@jessicah
Copy link
Author

Sorry, yes, I will get back to it; laptop died, and haven't gotten around to setting up on my other machine.

@Susurrus
Copy link
Contributor

We're about to cut a new release soon and I'd love to say that we fixed up Haiku support. So whenever you have time I'd love to merge this.

BTW what's your involvement with Haiku? You run it as a daily driver or just want to play with Rust on Haiku?

@jessicah
Copy link
Author

Okay, I'll see if I can get to it this week.

I'm core committer/maintainer. Mainly working on rust so can run librespot on Haiku :p

@Susurrus
Copy link
Contributor

Susurrus commented Dec 4, 2017

@jessicah Any updates on this? We've had a bunch of syntax changes happen (like from #801), so this will likely need a good rebase. If you don't have time to do this in the near future, I'd rather close this and you can re-submit a PR whenever some time frees up for you.

@Susurrus
Copy link
Contributor

I'm going to go ahead and close this, @jessicah, as it's been over 2 months since your last update. Please either reopen this PR or submit a new one that's been rebased on HEAD as a lot has changed since this was opened.

@Susurrus Susurrus closed this Jan 28, 2018
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.

3 participants