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

Move WinAPI functions from libc to std::sys::windows::c #26031

Closed
wants to merge 1 commit into from

Conversation

retep998
Copy link
Member

@retep998 retep998 commented Jun 5, 2015

The functions were only being used from libstd anyway.
As for the external version that people might depend on, people can either not cargo update their libc or just transition to winapi.
No types or constants were moved in this phase.

r? @alexcrichton

@retep998
Copy link
Member Author

retep998 commented Jun 5, 2015

Hmmm, it seems librustdoc::flock calls CreateFileW and CloseHandle. I'm going to see if I can rewrite it to use std::fs::File.

Also remove unused WinAPI functions

Signed-off-by: Peter Atashian <retep998@gmail.com>
@retep998
Copy link
Member Author

retep998 commented Jun 5, 2015

librustdoc::flock has been updated and the commit amended.

@alexcrichton
Copy link
Member

This is definitely a breaking change to the crates.io libc crate which needs to be considered. We can signal this with a semver-incompatible version bump (e.g. moving to 0.2), but I would personally rather hold off on making these sorts of breaking changes until we have a more comprehensive design for liblibc. I think that "just removing all the function definitions" and not dealing with the types, for example, may not be the best way to approach this.

Could you elaborate on the rationale for this? I'm interested to hear what you think the long-term story for these bindings to be.

cc @rust-lang/libs

@retep998
Copy link
Member Author

retep998 commented Jun 5, 2015

The rationale is that libc is not trying to be a wrapper to WinAPI. Most of the WinAPI in libc was added so that it could be used in Rust. Considering the current policy is to put new WinAPI in std::sys::windows::c rather than libc, it seemed like a good idea to clean out all the existing WinAPI from libc.

I'm not really sure how I envision libc's future since it is trying to be multiple things at once, like libc bindings and posix bindings, but neither is it attempting to include everything possible. The things in libc are a conglomerate of whatever was needed for Rust at some point in time, and are by no means comprehensive. For people who want posix and linux APIs there's things like nix, and for people who want WinAPI there is winapi.

I only did functions for now as more of a proof of concept to get feedback. I'd be happy to handle types and constants as well in this PR if doing everything at once is a better idea rather than splitting it up.

@nagisa
Copy link
Member

nagisa commented Jun 5, 2015

IMO at least liblibc in its current form – trying to bind all platforms – should not exist at all and cargo should instead allow depending on different crates per-platform. I.e. you would depend on liblibc-gnu on GNU systems, liblibc-bsd on BSD, liblibc-apple on OS X, winapi packages on windows etc.

@sfackler
Copy link
Member

sfackler commented Jun 5, 2015

It seems rather unfortunate to force any crate that wants to use well supported libc or POSIX functionality to explicitly enumerate all unixy systems in existence.

@nagisa
Copy link
Member

nagisa commented Jun 5, 2015

@sfackler

[dependencies]
posix = "1.0"

which can then depend on whatever liblibc it wants and export posix APIs only. Anyway, liblibc is certainly not a place for winapi.

@alexcrichton
Copy link
Member

I definitely agree that the design of liblibc needs to be re-thought, but I don't want to hastily go into a situation we're not prepared for. I think at this time we're not prepared for a sharding like @nagisa mentions, and I agree that this is leading towards that direction.

The alternative that I can think of is to continue the direction of liblibc being a "this is the system's API interface" kind of crate. There's definitely pros and cons in both directions, but it's something at least to consider.

@bors
Copy link
Contributor

bors commented Jun 28, 2015

☔ The latest upstream changes (presumably #26601) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton
Copy link
Member

I'm going to close this for now because it is a breaking change to liblibc and I'm not sure we're ready to commit to this path just yet. I suspect that a dive into redesigning liblibc will likely take this path, but for now it may be a bit premature to start moving everything around.

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.

5 participants