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

Should O_DIRECTORY be excluded for solarish platforms? #2273

Closed
travispaul opened this issue Dec 22, 2023 · 5 comments
Closed

Should O_DIRECTORY be excluded for solarish platforms? #2273

travispaul opened this issue Dec 22, 2023 · 5 comments

Comments

@travispaul
Copy link
Contributor

While tracking down a build error a couple of crates deep I've found that O_DIRECTORY is omitted on nix for solarish systems:

nix/src/fcntl.rs

Lines 99 to 100 in 2c6bbc4

#[cfg(not(solarish))]
O_DIRECTORY;

However, it is defined in fcntl.h

I've [patch]ed my way around the issue for the moment but I'm unsure if this is accidental or there is a good reason for it so opening this issue as not to forget over the holiday.

ping @jclulow, @jasonbking, @hadfl (saw that you folks worked on these changes as part of /pull/1394)

@jclulow
Copy link
Contributor

jclulow commented Dec 22, 2023

I believe support for O_DIRECTORY was added via illumos bug 9965 in early 2020, and we try not to expose things immediately in Rust programs so that software can continue to build and work on OS bits that are at least a couple of years old; e.g., OmniOS has an LTS release that lasts a few years at a time. I believe it's probably been long enough in this case to expose it now, though.

@travispaul
Copy link
Contributor Author

@jclulow thank you for clarifying, that makes sense.

@SteveLauC
Copy link
Member

From your guys' discussion, seems that we should still exclude it from Solarish OSes for now?

@jclulow
Copy link
Contributor

jclulow commented Dec 23, 2023

I can't speak for Solaris, but it seems reasonable to me to expose it for illumos systems at this point.

@travispaul
Copy link
Contributor Author

This has been resolved in /pull/2275, thanks all!

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

No branches or pull requests

3 participants