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

Display all platform-specific documentation on docs.rs #3076

Merged
merged 4 commits into from
Jan 4, 2024

Conversation

madsmtm
Copy link
Member

@madsmtm madsmtm commented Sep 1, 2023

This required a bit of refactoring regarding platform privacy, but all of it for the better.

  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users

@madsmtm madsmtm added S - docs Awareness, docs, examples, etc. S - platform parity Unintended platform differences S - maintenance Repaying technical debt labels Sep 1, 2023
@madsmtm madsmtm requested review from notgull and removed request for daxpedda, msiglreith and kchibisov September 1, 2023 10:45
CHANGELOG.md Outdated
@@ -8,6 +8,7 @@ And please only add new entries to the top of this list, right below the `# Unre

# Unreleased

- **Breaking:** No longer export `platform::x11::XNotSupported`.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@notgull I couldn't see a reason why this was exported, so I removed it, but maybe you can shed some light on it?

Copy link
Member

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this could become annoying to maintain, but generally a really good change for users!

src/lib.rs Outdated Show resolved Hide resolved
@kchibisov
Copy link
Member

I don't quite get the motivation behind this, it could be really confusing to show it all the time, given that you should pay attention to metadata, like where the function actually works.

I guess the only argument pro this is that it's easier to find docs for other targets, but I'd expect when you work with cross platform code you open docs for the target you want, since while winit can do such a thing with doc most stuff in projects doesn't have docs and internal stuff is obviously missed from the docs.

@madsmtm
Copy link
Member Author

madsmtm commented Sep 3, 2023

Motivation

You get to see immediately what functionality is available, without having to select on docs.rs the platform you're currently working on.

This would probably have gone a long way towards the user here finding a solution for their problem quicker.

The way the platform module looks on docs.rs now:
now

After this change:
after

@kchibisov
Copy link
Member

I mean, if they do cross dev, they should open docs with the correct target in the end. I'm just afraid to maintain that and work with such docs because I'm sort of really used to how it works based on target and I use the docs based on target.

The docs.rs even has a field to show docs per target, which sort of implies that they work for this target, but if we always show all the docs you can't really rely on your target picking.

Like for example I don't even want to see docs for macOS when writing code for linux, like why would I even have to read them and in my search, if they are just irrelevant for me?

@madsmtm
Copy link
Member Author

madsmtm commented Sep 3, 2023

Hmm, I definitely get your points! I think the best answer to that would be that the rustdoc team has implemented this feature because it's useful for users. I actually thought I could find a lot of examples of this, but it's quite hard since a lot of Rust code is cross-platform. (I also found examples of crates that don't do this, so take it with a grain of salt). But at least two examples:

Myself, I've often wished libc did this, I find it really difficult to know the exhaustive list of platforms e.g. libc::pthread_main_np is available on.

(If you're developing locally, you could always just do cargo doc --open, at least for now it'll only show docs for your own target/feature combinations. It's in any case always the best option, since it will show it for exactly your target, instead of just a target that's "close enough" (e.g. for winit I'd have to pick x86_64-apple-darwin on docs.rs even though I'm currently using aarch64-apple-darwin)).

@kchibisov
Copy link
Member

@madsmtm you can just pass --target to like anything and get the docs you want. If something. It's a bit weird when it comes to discoverability, indeed, but it's not like if we change something the ecosystem will change.

Also, have you looked into how rust does the docs in stdlib? They have #[doc(cfg(windows))] stuff and it somehow shows, but tells me explicitly that the thing is only available with windows right when I try to look into stuff. Though, maybe rust just builds docs differently?

@daxpedda
Copy link
Member

daxpedda commented Sep 4, 2023

I think "I'm not used to seeing the other platforms in the documentation" should be trumped by "increased discovery of platform-specific APIs in a cross-platform library".

About the maintenance issue: @madsmtm has done good effort in putting this together and it seems quite straightforward to me. The way Winit is structured lends itself incredibly well to this system, because we have separated the files so neatly.

I'm still strongly in favor of this change. But I acknowledge this comes with a maintenance burden, which I believe considering the benefits and the current structure of Winit is negligible.

Cc rust-lang/rust#1998.

@kchibisov
Copy link
Member

@madsmtm the issue I have after this change, is that it looks like this:

picture

So I have no information whatsoever to determine what is available for me and what is not. I was just using cargo doc and that's what I use daily to see the docs.

@kchibisov
Copy link
Member

Yeah, you need nightly for that and doc_auto_cfg, so the change is not applicable for us.

@daxpedda
Copy link
Member

daxpedda commented Sep 7, 2023

So I have no information whatsoever to determine what is available for me and what is not. I was just using cargo doc and that's what I use daily to see the docs.

I agree, that is a problem.

@madsmtm shouldn't we be able to only expose all other platforms when we also require doc_auto_cfg? E.g. with cfg(docsrs)?

daxpedda referenced this pull request Oct 24, 2023
This re-works the portable `run()` API that consumes the `EventLoop` and
runs the loop on the calling thread until the app exits.

This can be supported across _all_ platforms and compared to the
previous `run() -> !` API is now able to return a `Result` status on all
platforms except iOS and Web. Fixes: #2709

By moving away from `run() -> !` we stop calling `std::process::exit()`
internally as a means to kill the process without returning which means
it's possible to return an exit status and applications can return from
their `main()` function normally.

This also fixes Android support where an Activity runs in a thread but
we can't assume to have full ownership of the process (other services
could be running in separate threads).

Additionally all examples have generally been updated so that `main()`
returns a `Result` from `run()`

Fixes: #2709
@MarijnS95
Copy link
Member

MarijnS95 commented Oct 24, 2023

Looking forward to seeing this, especially in light of #3178, though hope it doesn't conflict too badly 😅

E.g. there is some docs pointing from run_on_demand (non-wasm!) to a wasm-only spawn() feature/function, which seems a bit confusing.

@madsmtm
Copy link
Member Author

madsmtm commented Dec 24, 2023

shouldn't we be able to only expose all other platforms when we also require doc_auto_cfg? E.g. with cfg(docsrs)?

Apologies for the delay, I've done so now.

@madsmtm madsmtm changed the title Doc: Always display platform-specific documentation Display all platform-specific documentation on docs.rs Dec 24, 2023
Copy link
Member

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for doing this, this is so cool!

src/platform/web.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/platform/web.rs Show resolved Hide resolved
src/platform/android.rs Outdated Show resolved Hide resolved
Copy link
Member

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

madsmtm and others added 4 commits January 4, 2024 12:48
This required a few ugly hacks, such as making some #[doc(hidden)] empty structs on platforms where we expose some external dependency - but I tend to think that's a somewhat good thing, since it makes it more explicit that we have a public dependency here.
Co-authored-by: daxpedda <daxpedda@gmail.com>
@daxpedda daxpedda merged commit 42dbc47 into master Jan 4, 2024
51 checks passed
@daxpedda daxpedda deleted the docsrs-all-platforms branch January 4, 2024 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S - docs Awareness, docs, examples, etc. S - maintenance Repaying technical debt S - platform parity Unintended platform differences
Development

Successfully merging this pull request may close these issues.

4 participants