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

Build is broken with default features disabled #3174

Open
Ralith opened this issue Oct 22, 2023 · 16 comments
Open

Build is broken with default features disabled #3174

Ralith opened this issue Oct 22, 2023 · 16 comments

Comments

@Ralith
Copy link
Contributor

Ralith commented Oct 22, 2023

winit fails with errors including, but not limited to,

error: The platform you're compiling for is not supported by winit
  --> /home/ralith/.cargo/registry/src/index.crates.io-6f17d22bba15001f/winit-0.29.2/src/platform_impl/mod.rs:67:1
   |
67 | compile_error!("The platform you're compiling for is not supported by winit");
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

when built with default-features = false and no features enabled. This configuration is desirable for e.g. libraries that translate winit event types but which shouldn't have an opinion on enabled backends or configuration thereof.

@king0952
Copy link

king0952 commented Oct 23, 2023

Add these lines in Cargo.toml, it currently works for me:

[dependencies.winit]
# version = "*"
version = "=0.28.7"
features = ["default"]

And then cargo update

  Updating crates.io index
  Removing android-activity v0.5.0
  Removing as-raw-xcb-connection v1.0.0
  Removing atomic-waker v1.1.2
  Removing block-sys v0.2.0
  Removing block2 v0.3.0
Downgrading calloop v0.12.3 -> v0.10.6
  Removing calloop-wayland-source v0.2.0
  Removing core-graphics v0.23.1
  Removing cursor-icon v1.0.0
  Removing foreign-types v0.5.0
  Removing foreign-types-macros v0.2.3
  Removing foreign-types-shared v0.3.1
  Removing gethostname v0.3.0
  Removing icrate v0.0.4
Downgrading memmap2 v0.9.0 -> v0.5.10
  Removing ndk v0.8.0
  Removing ndk-sys v0.5.0+25.2.9519653
    Adding nix v0.25.1
  Removing num_enum v0.7.0
  Removing num_enum_derive v0.7.0
  Removing objc-sys v0.3.1
  Removing objc2 v0.4.1
  Removing objc2-encode v3.0.0
  Removing polling v3.2.0
  Removing raw-window-handle v0.6.0
Downgrading sctk-adwaita v0.7.0 -> v0.5.4
Downgrading smithay-client-toolkit v0.18.0 -> v0.16.1
Downgrading tiny-skia v0.11.2 -> v0.8.4
Downgrading tiny-skia-path v0.11.2 -> v0.8.4
    Adding vec_map v0.8.2
  Removing wayland-backend v0.3.2
Downgrading wayland-client v0.31.1 -> v0.29.5
    Adding wayland-commons v0.29.5
  Removing wayland-csd-frame v0.3.0
Downgrading wayland-cursor v0.31.0 -> v0.29.5
Downgrading wayland-protocols v0.31.0 -> v0.29.5
  Removing wayland-protocols-plasma v0.2.0
  Removing wayland-protocols-wlr v0.2.0
  Removing wayland-scanner v0.31.0
Downgrading wayland-sys v0.31.1 -> v0.29.5
  Removing web-time v0.2.2
  Removing winit v0.29.2
  Removing x11rb v0.12.0
  Removing x11rb-protocol v0.12.0
  Removing xkbcommon-dl v0.4.1
  Removing xkeysym v0.2.0

@kchibisov
Copy link
Member

when built with default-features = false and no features enabled

this is expected, because you've disabled both x11 and wayland and build for linux. What display server are you going to run this way? You need either something providing wayland or x11.

@Ralith
Copy link
Contributor Author

Ralith commented Oct 23, 2023

As I explained in the original post:

This configuration is desirable for e.g. libraries that translate winit event types but which shouldn't have an opinion on enabled backends or configuration thereof.

Binary crates are not the use case. The status quo makes it impossible to compile library crates depending on winit without forcing decisions that should be left to binaries.

@kchibisov
Copy link
Member

Winit can't work without either x11 or wayland because how it's designed. You simply want to build docs without internal dispatching somehow just by leaving top-level definitions. If you want something like that you could try to send a patch, but I have no desire to maintain support for unusable platform configuration. Could just build macOS docs at this point.

@Ralith
Copy link
Contributor Author

Ralith commented Oct 23, 2023

I'm not trying to build docs. I'm trying to make a library pass CI without forcing decisions on downstream. Please reopen this issue.

@kchibisov
Copy link
Member

But you understand that the library is unusable? They can't use none of the winit APIs without either x11 or wayland, why we should support such a case? winit by default forces both wayland and x11, so the application works, since winit a cross platform toolkit, so we should enable all the platforms we can.

I don't want winit to support building without both, since it'll require us annotate a lot of code to suppress rust warnings, because suddenly nothing works. We need especial code handling to panic when you try to run EventLoop because you don't have any platform anymore. I don't want to deal with issues that someone tries to run winit without features, I'm not sure how it's not clear, because winit can't function without it.

It can theoretically, build, but why should we support something that can't function?

@Ralith
Copy link
Contributor Author

Ralith commented Oct 23, 2023

They can't use none of the winit APIs without either x11 or wayland, why we should support such a case?

Downstream users will enable one or more backends. Libraries should not make that decision for them.

We need especial code handling to panic when you try to run EventLoop because you don't have any platform anymore.

EventLoop::new already returns a Result. No backends being available seems like a good reason for construction to fail gracefully; no need for user-accessible panics.

It can theoretically, build, but why should we support something that can't function?

Because it allows libraries to depend on winit without having to choose between being forcing decisions they don't care about on their users or having hopelessly broken CI/rust-analyzer/etc.

@kchibisov
Copy link
Member

I don't want to continue in this conversation, it's not like you or I will change my mind. I want a library to have a sensible set of defaults. It's not like we don't let users opt-out and it's not like

having hopelessly broken CI/rust-analyzer/etc

Fix these tools, not complain that library enabled features, which don't even link to system libraries by default because we pick dlopen based linked.

Because it allows libraries to depend on winit without having to choose between being forcing decisions they don't care about on their users or having hopelessly broken CI/rust-analyzer/etc.

I'm not arguing against having non-working code, sorry.

Feel free to work on that if you want to, but be aware that I won't maintain non-functional mess it'll create and lots of cfg to get past warnings, error checking. Having winit working on all platforms, except for x11/wayland sounds insane to me.

Maybe I should just remove X11 at this point? I don't see why should I keep it around, then I don't need a feature for Wayland and everyone wins. Sounds like a plan.

@sanbox-irl
Copy link

in https://github.com/imgui-rs/imgui-rs we used to get spurious errors for this ALL THE TIME until we went with default-features=false. We still get tons of user errors and reports (imgui-rs/imgui-rs#742), but we're not quite up to the latest winit, so I'm not sure if things have changed.

Letting us build in CI and pass these bugs into user space would be great. We are an example of a library which users winit types but doesn't actually construct the backend ourselves, so we don't care what features winit is being used with

@kchibisov
Copy link
Member

@sanbox-irl how you do you work outside of linux then? Do you know that you link system libraries on them? I'm pretty sure no-one understands that and just want to go on a rant how bad am I for forcing users into a working library.

@sanbox-irl
Copy link

sanbox-irl commented Oct 23, 2023

So Imgui-rs is a library, not a binary. So we don't need to be executable. Users do:

winit = { version = "whatever", features = [
    "x11",
] 

If imgui-winit itself specifies, ie, features = "wayland", then users wouldn't be able to use imgui-winit + winit, features = "x11". But if instead imgui specifies no default features, then cargo will resovle them to the same crates, and so it all works out.

Also I'm sorry that you feel like we're "ranting" at you -- I think though that Ralith's point is respectfully delivered. If this isn't the way winit wants to go, I understand, but this is a valid concern to raise IMO!

@cwfitzgerald
Copy link

cwfitzgerald commented Oct 23, 2023

Please don't remove x11 - we need it for renderdoc support.

I can see the argument for allowing no default features, as it stands, all libraries (egui-winit, imgui-winit, etc) that depend on winit, needs to replicate the x11/wayland feature, with defaults, so the end binary user can disable both winit and the crate's default features and enable just one backend if they do desire.

On the other hand if this will cause undue maintence burden on winit, it can be worked around (see above) so it's not the end of the world.

@Ralith
Copy link
Contributor Author

Ralith commented Oct 23, 2023

I want a library to have a sensible set of defaults.

It is fine to have sensible defaults. That is not relevant to the problem here.

Fix these tools

How, exactly, do you expect rustc/rust-analyzer to be fixed to successfully compile code that is malformed with default features disabled?

Feel free to work on that if you want to, but be aware that I won't maintain non-functional mess it'll create and lots of cfg to get past warnings, error checking

I'd be happy to have a go at it. I suspect it won't be as messy as you fear since the cost for backends being cfg'd out has already been paid, but I guess we'll see.

Having winit working on all platforms, except for x11/wayland sounds insane to me.

I'd be just as happy to have all backend code cfg'd out for libraries with default-features = false, but I agree that the extra effort to do so on single-backend platforms would be a bit pointless.

@kchibisov
Copy link
Member

Please don't remove x11 - we need it for renderdoc support.

I just don't understand desire for non-working library.

I can see the argument for allowing no default features, as it stands, all libraries (egui-winit, imgui-winit, etc) that depend on winit, needs to replicate the x11/wayland feature, with defaults, so the end binary user can disable both winit and the crate's default features and enable just one backend if they do desire.

Exactly, it'll just spread and most users don't care. The only reason those features do exist in the first place because alacritty users asked a lot to have a way to compile without X11 at all.

I'd also point that what you want to do here is very different to how it should be done.

You want a way to have winit types in your downstream, not winit itself. Otherwise if you want winit itself you want to be consistent and we should have features like

macOS
iOS
Android
Windows
X11
Wayland
Web
Redox

And some backends have dl/non-dl varants, and Wayland has decorations variants. So in the end we end up with ~15 features, 9 of them are essential to have something working for the majority of users.

@kchibisov
Copy link
Member

I'll leave this up to you if you want to work on something like that, but it should meet the following:

  • cfg guards to disable rust warnings could only be in a one place, see glutin on how to do that.
  • You must provide a features = full and it must be advertised everywhere.
  • The CI should test feature combinations of what you've decided to provide.

Then I might consider it into winit, even when I don't understand the end goal with all of that when what it really sounds to me is that you want a winit-types, because libraries accessing graphics work with raw-window-handle crate, not with winit crate.

Toolkits have a core without winit itself, but have integrations to work with winit, those integrations should pick default winit features, imho, otherwise users won't be able to run example. Do you want to tell the user to enable x11 each time they run example? Do you think something like that makes sense, where windows will just work?

I'd also note that I have to deal with WAYLAND IS FUNDAMENTALLY BROKEN comments in winit for over 2 years because we haven't showed a window in examples, even tough I said that before. I really don't want to go through all of that again and read what I see sometimes, thanks.

@Ralith
Copy link
Contributor Author

Ralith commented Oct 23, 2023

I'll leave this up to you if you want to work on something like that

Thanks, I'll have a go as time permits.

you want a winit-types

This would solve some instances of the problem. However:

  • It seems likely to be a more disruptive change to winit
  • It might not address cases where a library wants to borrow Window, EventLoop, or an ELWT, as those are more difficult to isolate. This covers my motivating use cases, yakui-winit and yakui-app.
  • There may be other unforeseen reasons to depend on winit in a (non-framework) library

integrations should pick default winit features, imho, otherwise users won't be able to run example

I agree that libraries which can be used by binaries which do not themselves have a direct dependency on winit should enable supported backends by default, if not unconditionally. However, if a binary needs a direct dependency on winit regardless (common for lightweight integration crates), then winit's own default features take effect, providing a good user experience.

I have to deal with WAYLAND IS FUNDAMENTALLY BROKEN comments in winit for over 2 years

I agree that avoiding confusion downstream is paramount. Leaving the default features as-is should prevent any effect on the overwhelming majority of users: today, nobody building for Linux is compiling with default-features = false, so changes that only affect that case will not be disruptive. In the event that a framework-style library is released which fails to enable sensible defaults, I think an intelligible error from EventLoop::new at runtime will be at least as good an experience as the current wall of errors at compile time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

6 participants