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

Use custom bindings instead of winapi crate. #89

Closed
wants to merge 1 commit into from

Conversation

RazrFalcon
Copy link

@RazrFalcon RazrFalcon commented Aug 10, 2019

The winapi crate requires winapi-*-pc-windows-gnu crates, which are pretty heavy (100MB). This blow ups the vendored archives size. Which is important for such a widely used crate.

There are only two ways to fix this:

This patch doesn't make this library less safe since winapi crate doesn't contain any logic. We simply copy-pasting winapi crate definitions.

This patch doesn't break comparability/api since it affects only the internal implementation.

The only remaining question is portability. The winapi crate does a lot of linking magic, but I'm not sure if we need it at all. UPD since all tests are passed, looks like we don't need it anyway.

@BurntSushi
Copy link
Collaborator

What are the winapi-*-pc-windows-gnu crates providing, and why don't we need them here?

I note that it seems like you can just tell winapi not to bundle the libraries (which only happens on the gnu targets anyway): https://github.com/retep998/winapi-rs/blob/0.3/x86_64/build.rs#L10

@RazrFalcon
Copy link
Author

I'm not sure why do we need them, honestly. Looks like some kind of workaround for mingw.

And I'm not sure what this flag does, but it doesn't impact cargo build and cargo vendor. winapi-*-pc-windows-gnu will still be downloaded and built, but not linked.

@BurntSushi
Copy link
Collaborator

cc @retep998 - There is some desire to avoid pulling in the winapi-*-pc-windows-gnu crates, by dropping the winapi dependency and inlining the bindings. What downsides does this have?

While I don't maintain memmap, I do maintain several crates that do use winapi, so I'd like to understand this issue more fully.

@est31
Copy link

est31 commented Aug 10, 2019

@BurntSushi see this comment by @retep998 as well as the motivation section of the raw-dylib RFC for use cases of those crates.

@BurntSushi
Copy link
Collaborator

I don't think any of that really answers my question. Or at least, I am not knowledgeable enough to use those details to extrapolate to ultimate trade offs. For example, it sounds to me like if we just declare the bindings ourselves and drop winapi completely (like this PR does), then it sounds like something on the gnu targets will break. The extent and nature of that breakage isn't clear to me.

@danburkert
Copy link
Owner

Thanks for the PR and good discussion. I agree with @BurntSushi - it would be great to understand the issue fully before taking on this maintenance burden. Additionally, my gut is that it would be relatively rare for memmap to be the only crate depending on winapi in any given applications dependency hierarchy, so fixing it here isn't very high impact.

@RazrFalcon
Copy link
Author

RazrFalcon commented Aug 10, 2019

@danburkert I'm not sure if you can list all crates on crates.io that depend on a particular crate, so it's hard to say how often memmap is used with winapi. Personally, I need this for my fairly large crate, and this will be the only dependency that uses winapi. And it's pretty important, since it reduces the vendored archive size in half (7MB -> 3.5MB). And in 95% of the cases this archive will be used by linux distros that doesn't care about winapi at all.

But I agree that we should understand why winapi uses those libraries at all and how this will impact memmap.

@est31
Copy link

est31 commented Aug 10, 2019

@RazrFalcon if you don't need windows anyway then maybe this could help: rust-lang/cargo#7058

@RazrFalcon
Copy link
Author

@est31 No, all platforms are required anyway.

@retep998
Copy link
Contributor

retep998 commented Aug 11, 2019

  • The pc-windows-gnu targets come with a bundled subset of MinGW which means I cannot assume a full MinGW toolchain exists. Many functions come from system import libraries not included in the bundled subset of MinGW.
  • Even when the MinGW import library I need exists, it is usually woefully out of date and does not contain newer functions.

Therefore winapi has target specific dependencies which bundle import libraries that are up to date. I cannot remove them as dependencies of winapi without breaking very real use cases where people try to build something using the pc-windows-gnu toolchain and get linker errors because a library doesn't exist or a symbol is unresolved. Until the raw-dylib feature is implemented and stabilized, there is nothing that winapi can do. I already try to minimize the impact as much as possible by making them target specific.

In this specific case because you are limiting yourselves strictly to functions included in the bundled subset of MinGW it will work without winapi's bundled import libraries. Also because you are only using system libraries already linked to by std you don't need to do any linking magic.

@RazrFalcon
Copy link
Author

@retep998 Thanks for clarification. Maybe this should be added to the winapi readme?

RazrFalcon added a commit to linebender/resvg that referenced this pull request Jan 10, 2020
This one doesn't depend on winapi.
danburkert/memmap-rs#89
@BurntSushi
Copy link
Collaborator

My suggestion here is to keep the dependency on winapi. It sounds like the stated downside here is a few extra MB of disk space. IMO, this is not worth it. In part because of @danburkert's point where winapi is a very common dependency. It's hard to build a Rust application with Windows support without it. IMO, a few extra MBs of disk space is not an urgent issue that should be fixed by the crate ecosystem. Instead, the tooling should find a way to fix this.

@RazrFalcon
Copy link
Author

@BurntSushi I've already made a fork, so it's no longer a problem, at least for me.

And no, "a few extra MB of disk space" is very important to me.

@est31
Copy link

est31 commented Feb 21, 2020

There are two much better alternatives to this:

  • The best alternative would be to implement the raw-dylib RFC. Tracking issue for RFC 2627: #[link(kind="raw-dylib")] rust-lang/rust#58713
  • The second best way would be to find out whether the .lib files could be preprocessed to be smaller. winapi/rustc only needs a small fraction of the info contained in them, so figuring out how to remove the redundant parts can likely achieve great gains in size. It's the second best way because once the raw-dylib RFC is being implemented, it won't be needed any more.

@BurntSushi
Copy link
Collaborator

I'm going to be helping out with maintenance for this crate. On balance, I think this PR probably isn't worth it. Thanks though!

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