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

RFC: Provide pre-built version of this library #432

Closed
Jasper-Bekkers opened this issue Jan 22, 2021 · 32 comments
Closed

RFC: Provide pre-built version of this library #432

Jasper-Bekkers opened this issue Jan 22, 2021 · 32 comments
Labels
enhancement New feature or request

Comments

@Jasper-Bekkers
Copy link

First off: thanks for a nice addition to the Rust ecosystem, it's really wonderful to see Microsoft actively try to support what is going on and provide support for the win32 apis.

However as a maintainer of several code-generation crates I've found that it's usually significantly nicer for the user of my crates to provide a pre-built version of the wrappers that I'm exposing. This has a few benefits

  • build.rs is a serial dependency and very slow for more complex projects since cargo has to stall building code further down the line, and it has to wait for code-gen crates like syn and quote to have been built before it can run a build.rs.
  • Right now the recommended workflow is for everybody else to create a pre-built version, however, this means making windows-rs types part of your public interface will lead to incompatibilities between crates, whereas before you could make sure that everything is on the same version through cargo deny for example.

To see the impact of building this crate, one can run cargo +nightly build -Ztimings to see the build dependency graph and this crate's impact on it (for example on the minesweeper-rs example that you've provided it looks like the vast majority of it's ~70s build time is spent in code-gen related things).

If a pre-generated version of this crate becomes available (or nicers: this crate switches into a pre-built library) please add feature toggles similar to how winapi-rs does things, this lets the cargo feature resolver make sure that the minimal set of features is built across the entire dependency tree.

@rylev
Copy link
Contributor

rylev commented Jan 22, 2021

I'm personally in favor of that. The API surface is huge though so it's not exactly clear how best one would offer a pre-built version. So, some additional questions:

  • Should it be a single crate or multiple crates?
  • If multiple, how should they be split?

@nagisa
Copy link

nagisa commented Jan 22, 2021

I don't think it is terribly wrong to give an ability to pre-generate and include in crates just the bindings they need. As you mention it can lead to incompatibilities if these are exposed in the public interface of the crates, but in my experience the need to do so is pretty rare.

@repi
Copy link

repi commented Jan 22, 2021

In my experience the winapi 0.3 approach is proven to work well, single huge crate but with features for all areas/modules that are disabled by default so each crate using it has to opt in to the feature they do want to use. This compiles nicely and the cargo additive feature support makes sure winapi is built once with all the features that all dependencies have enabled.

In our codebase we have ~50 direct usages from our own and third party crates to winapi (0.3) and it builds really fast and is easy to use. Think almost none of those crates expose the winapi types as public symbols, but some may/could and it would still work well also.

@kennykerr kennykerr added the enhancement New feature or request label Jan 22, 2021
@thomcc
Copy link

thomcc commented Jan 22, 2021

In my experience the winapi 0.3 approach is proven to work well, single huge crate but with features for all areas/modules that are disabled by default so each crate using it has to opt in to the feature they do want to use. This compiles nicely and the cargo additive feature support makes sure winapi is built once with all the features that all dependencies have enabled.

IME it hurts in large workspaces, since cargo test -p <onecrate> and cargo test -p <anothercrate> will end up with different feature combos for winapi, which will result in rebuilding it and all dependencies (which in practice is a lot of your workspace dep tree) each time. You see things like https://searchfox.org/mozilla-central/source/build/workspace-hack/Cargo.toml sometimes that exist to mitigate it (which is only realistically viable if you're not on crates.io, so is not that common).

Personally I'd prefer to use it in a pre-generation context, which avoids all these issues.

@repi
Copy link

repi commented Jan 22, 2021

IME it hurts in large workspaces, since cargo test -p <onecrate> and cargo test -p <anothercrate> will end up with different feature combos for winapi, which will result in rebuilding it and all dependencies

Yup that is true and a fair point, we do have a fairly large workspace (~550 crate dependencies) but less common to build and test subparts of it. But definitely does happen and is a common cargo workflow in general.

@Lokathor
Copy link

What should probably happen is a generator / default-generated split.

The generator at work here is amazing work, and it's fine for it to just be a thing that exists on its own.

However, many people want to use a pre-generated form, chunked up by feature flags or whatever. Just offer that as a crate separate from the generator itself.

Hek, just run this generator and then use the output as the content of winapi (in a 0.4 version)

@thomcc
Copy link

thomcc commented Jan 23, 2021

less common to build and test subparts of it.

Yeah, there's a size of workspace where this becomes untenable.

@roblabla
Copy link
Contributor

roblabla commented Jan 23, 2021

IME it hurts in large workspaces, since cargo test -p and cargo test -p will end up with different feature combos for winapi, which will result in rebuilding it and all dependencies (which in practice is a lot of your workspace dep tree) each time.

This feels like something that could be fixed in cargo itself, e.g. through the deduplicate workspace information RFC eventually. Through this RFC, you could specify the features once in the workspace root, and all the packages in your workspace would end up using a common build.


Personally, I can't use this crate without a pregenerated version being made available. Having every single crate generate their own bindings through windows-rs feels like a recipe for disaster in terms of compile-time regressions compared to the current winapi-rs approach.

@thomcc
Copy link

thomcc commented Jan 26, 2021

This feels like something that could be fixed in cargo itself, e.g. through the deduplicate workspace information RFC eventually

It feels to me like without a major redesign, it will always be something of an issue, but perhaps I'm too pessimistic.

Having every single crate generate their own bindings through windows-rs feels like a recipe for disaster in terms of compile-time regressions compared to the current winapi-rs approach.

Yes, I agree here, unless the pregeneration approach is widely taken, which it probably won't be, since it will always be a bit too work intensive. That said, I'd really like it as an option (Plenty of crates already do this by hand, it would be valuable to improve things for them).

@kennykerr
Copy link
Collaborator

kennykerr commented Feb 26, 2021

Sorry for the delay in responding. This is an important issue to us. In fact, we had a meeting to discuss this topic just yesterday. This matters for some of our internal teams as well. We would like to provide pre-generated bindings as a collection of crates, but there are pros and cons to doing so today. Here are a few issues to consider:

  1. The scope of the bindings that the windows crate can generate far exceed those of the hand-written winapi crate. Since we tap into metadata, we can generate bindings for the entire set of Win32, COM, and WinRT APIs. We thus have scalability issues that are not as obvious in a hand-written approach.
  2. The Windows API developed over a number of decades and is very large but not well layered. The Windows API is organized as a set of logical namespaces. One approach is to model those individual namespaces as individual pre-generated crates. That would be roughly 540 crates.
  3. One challenge with this approach is that Rust does not support the inherent dependencies implied by the Windows API. Crate A may depend on crate B that depends on crate A. We are exploring ways to improve the Rust language and toolchain to able to support this.
  4. Another approach is to have one large crate that includes everything and use Rust features to limit what is compiled. The trouble is that "features" are pretty limited and we would end up with hundreds or even thousands of features and some features would depend on other features. Again, we would need to explore ways to improve Rust to able to support this more effectively.
  5. Another challenge is that the Rust compiler is very slow. It is many times slower than the C++ compiler. Part of the problem is the compiler’s single-threaded performance and part of it is the lack of multi-threading. We have people looking into improving the performance of the Rust compiler. @rylev has already had some success here.
  6. Another approach is to introduce a binary crate model where cargo would not need to compile each dependency from source, akin to C++ modules.
  7. Another approach is to change the windows crate’s default model of greedy dependency tracking and replace it with a more conservative approach that elides any methods on COM and WinRT interfaces that refer to types not specifically mentioned in the build macro (Consider non-greed dependency tracking option #567). The vtable slots would be preserved but the projected methods would be omitted. This has the potential of generating far less code and thus speeding up builds considerably. This has been done. 😉

So there are just a few challenges and ideas we're throwing around. The main benefit we see for pre-generated bindings is to allow crates to share common definitions of Windows types. This is something we would like to support, but there are technical hurdles to overcome. These are hard problems, and we look forward to working together to solve them.

@rylev
Copy link
Contributor

rylev commented Mar 29, 2021

I've been thinking more about this. I'd like to try to capture the pros and cons of pre-built crates as well as the dependencies. Some of this will be a repeat of what @kennykerr says above but hopefully this provides a bit more structure to think about this problem.

Pros

  • Possible compile time improvements
    • The lack of proc-macro code generation means less dependencies and and less work being done before main compilation happens.
    • The pre-generated crate can be shared among upstream dependencies meaning that the code needs to only be able to compiled once.
  • Common definitions
    • Not only can upstream dependencies rely on a single crate being compiled they can also share definitions meaning there won't be a case where two logically equal types can't interoperate since they are compiled by two different crates.

Cons

  • Hard to factor/organize
    • The Windows API surface is huge (much bigger than what winapi currently exposes) and only likely to get bigger as more definitions are added to the metadata. Deciding on how to break up the Windows API surface is not an easy problem.
    • You could separate by namespace but some namespaces are huge themselves and many depend on other namespaces in circular ways. We should do some analysis of the current metadata to understand the shape of this issue better, but this makes it unlikely that a many-pre-generated-crates solution would work.
    • Going down the single-crate path, we could use features like winapi does, but this is already pretty unwieldy in winapi and as I said above, a pre-generated crate would be much larger than winapi is.

Next steps

  • Try an experiment where we pre-generate the entire Windows API surface with each namespace being behind a feature flag.
    • We can then get feedback on how people thing using such a crate would be in practice, and we can do compilation tests to see how this affects compilation time.
    • We should also experiment with putting some things behind additional feature flags: e.g., Debug implementations should be behind a feature flag.

I definitely think no matter what happens we will keep on-demand generation as an option. It's extremely valuable in many situations. However, a pre-built crate may be an additional choice for those who want/need it.

More ideas

Potentially, we might be able to let these different approaches work well together. For instance, we could provide a Win32 crate which only contains definitions in the Win32 namespace. Then when the user is generating bindings outside this namespace, they could optionally let the code generator know that they want to depend on the Win32 definitions in the win32 crate instead of generating those definitions. There are issues with version management, but this is certainly a possibility.

After some testing, I found that the Win32 namespace has dependencies only on two other namespaces: Foundation and System. Luckily these namespaces do not depend on any others. So if we go down the route of a Win32 crate, we'll need to decide on some things:

  • Are Foundation and System their own separate crate?
  • Does this lock us into a world where Win32 cannot rely on any other namespaces? What happens if such a dependency is introduced in the metadata?

@Plecra
Copy link

Plecra commented Mar 31, 2021

Possibly silly idea: could the generator (since it has the full metadata) solve dependency cycles and generate a windows_core module to break them up? This would allow the namespaces to be exposed as individual crates.

@rylev
Copy link
Contributor

rylev commented Mar 31, 2021

@Plecra this is something that we could explore, but it seems that namespaces often have circular dependencies which would not be served well by a separate crate model. Also, even if this is possible now, we would essentially be committing to this model forever. What happens when new APIs come along that break this model? The Windows APIs have a lot bigger concerns than if the break the Rust bindings, so it's unlikely we would be able to prevent such a thing from happening.

Like I said above, perhaps we might be able to build a model that handles both pre-generated and non-pre-generated bindings. Then for some select namespaces, we can pre-generate the bindings if we feel comfortable enough that those namespaces work well in such a model.

@Lokathor
Copy link

Can you point out some of the circular dependencies that cause trouble? Because I think that enough of Win32 is non-circular that it's not a big deal if a few fringe cases have to be combined into a single loop of stuff that you add all at once.

@rylev
Copy link
Contributor

rylev commented Mar 31, 2021

@Lokathor I don't have an example off the top of my head. I will try to do some spelunking and find some. For win32 specifically (i.e., the things in the metadata under the win32 namespace), it doesn't seem like there are any dependencies currently outside of Foundation and System namespaces. So hopefully we can provide a win32 crate. The next step is to create such a crate and see how bad compile times are for each sub-namespace of the win32 namespace.

@kennykerr
Copy link
Collaborator

It would be insightful to use the metadata reader to model the dependencies. I would however be quite reluctant to favor some APIs over others. Ideally, we can find a solution that works for all APIs and not just some blessed APIs that happen to not have cyclic issues.

@Lokathor
Copy link

Yeah. Not to try and diminish the work on the other APIs also being supported, but I think at this time that most people are only using Win32, particularly because that's where the winapi support is at the moment.

My suggestion would be to coordinate with @retep998 about possibly having the next semver break of winapi (0.4) be generated via this generation system, and having it focus on just the win32 part of things. I think this would produce the least overall churn in the ecosystem.

@kennykerr
Copy link
Collaborator

kennykerr commented Mar 31, 2021

Here's a quick little dump of dependencies.

use std::collections::{BTreeMap, BTreeSet};

fn main() {
    let reader = gen::TypeReader::get();
    let mut deps = BTreeMap::<&'static str, BTreeSet<&'static str>>::new();

    for ns in reader.namespaces() {
        for et in reader.namespace_types(ns) {
            for def in et.dependencies() {
                if et.namespace() != def.namespace() && !def.namespace().is_empty() {
                    deps.entry(et.namespace())
                        .or_default()
                        .insert(def.namespace());
                }
            }
        }
    }

    println!("{:#?}", deps);
}

Output: https://gist.github.com/kennykerr/b27b6e803078336c2d55780109820c3e

@rylev
Copy link
Contributor

rylev commented Apr 1, 2021

I did some analysis on the dependency output that Kenny posted.

  • 315 total namespaces with at least one dependency.
  • 76 depend on only Windows.Foundation.
  • Win32 depends on Windows.Foundation and Windows.System. It is the only namespace dependent on Windows.System.
  • 55 namespaces depend only on Windows.Win32
  • There are a bunch on recursively dependent namespaces but they are always in the same top level namespace (e.g., Windows.Media.Core mutually recursively depends on Windows.Media.Playback)

We need the following two features to be able to experiment with this:

  • Add support for features in the build! macro
  • Add support for specifying that a namespace comes from a 3rd party crate and shouldn't be generated.

@Ciantic
Copy link

Ciantic commented Apr 11, 2021

I see that there are three strategies:

  1. If you are building a library which consumes or outputs WinAPI types, you should use pre-built windows-rs packages (this issue).
  2. If you are building a library that merely uses WinAPI types but does not expose them, you could bundle pre-generated bindings without macro dependencies.
  3. If you are building an application then you could use macro generated bindings.

Notice the case 2. If your library is merely using the bindings, it's wasteful to depend on macro generation because every user of the library would generate the same bindings. Thus it's cheaper for us all if the bindings are pre-generated and bundled as code for that kind of libraries.

@kennykerr
Copy link
Collaborator

Just a quick update - I've been working on this issue over the last week and have a working proof of concept. There are some rough edges but it definitely looks like this will be a viable solution before long.

@kennykerr
Copy link
Collaborator

kennykerr commented Oct 15, 2021

To summarize, developers will then be able to choose between:

  1. the build macro (like these samples)
  2. the generate macro (like this PR and the windows crate itself)
  3. the pre-built windows crate!

I just need the metadata folks to clean up the namespace dependencies to improve build performance, and a few other loose ends like nightly raw-dylib support.

@Jasper-Bekkers
Copy link
Author

Will a 3 proposed solutions be compatible with each other and share code / compile time?

@kennykerr
Copy link
Collaborator

"3" finally allows Windows types to be shared across crates in a stable way.

There are however still a lot of details to work out around how that will interoperate with code generated by build/generate for non-Windows APIs as well as with the implement macro for implementations.

@mwcampbell
Copy link

FWIW, I just switched my work-in-progress Windows implementation of AccessKit to use the new pre-packaged bindings. The only wrinkle was that I had to add the Win32_System_SystemServices feature to enable VARIANT.

What's blocking the publication of version 0.22.x on crates.io? AccessKit isn't yet on crates.io, so it's not yet a problem for me, but it would be useful to know.

@kennykerr
Copy link
Collaborator

That's great to hear! microsoft/win32metadata#710 should help with some of those bigger namespaces. If you find more glaring issues like that please do report them to the win32metadata project.

I plan to publish a crate today - I was just waiting for an exemption as the crate is a little larger than the max allowed.

I'll also close this issue as this feature is now available.

@kennykerr
Copy link
Collaborator

Published! https://crates.io/crates/windows

@MarijnS95
Copy link
Contributor

Thanks a lot @kennykerr! Casually releasing 0.22.1 without 0.22.0 😁

@kennykerr
Copy link
Collaborator

😄 I had originally planned to release 0.22.0 and got as far as the dependency crates and then the windows crate failed to publish due to the max upload size. By the time that was approved, I had made a few tweaks to the dependency crates and didn't want to risk having them out of sync.

@nico-abram
Copy link

The windows api link above gives a 404

@MarijnS95
Copy link
Contributor

@nico-abram That was a temporary prototype, the pregenerated bindings are now published right inside the windows crate! 🎉

@mwcampbell
Copy link

@nico-abram The pre-built API bindings are in the windows crate itself, in this repo. The updated readme shows how to use them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests