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

Fix building docs on Windows #6945

Merged
merged 6 commits into from
Nov 13, 2024
Merged

Fix building docs on Windows #6945

merged 6 commits into from
Nov 13, 2024

Conversation

jofas
Copy link
Contributor

@jofas jofas commented Oct 29, 2024

I believe this fixes #6625 by only building the io::bsd module on Unix machines, in accordance with #6625 (comment). Putting the #[cfg(unix)] outside of the cfg_aio! macro felt less intrusive, as the macro is also used in io::Interest.1 I haven't yet tested this on Windows (or via cross-compilation).

While creating this PR I checked out the build logs from docs.rs and saw that the two Windows builds there are broken as well. I want to replicate how docs.rs builds the docs for Windows and fix them, too, so starting this PR as a draft.

I wonder if building the docs on Windows would be a good idea to add to CI, to guard against future regressions?

Footnotes

  1. We should be able to build Interest on Windows with --cfg docsrs without problems.

@jofas jofas changed the title Build io::bsd module only on unix machines Fix building docs on Windows Oct 29, 2024
@mox692
Copy link
Member

mox692 commented Oct 29, 2024

I wonder if building the docs on Windows would be a good idea to add to CI, to guard against future regressions?

Yes, this sounds good to me.

@mox692 mox692 added the T-docs Topic: documentation label Oct 29, 2024
@Darksonn Darksonn added A-tokio Area: The main tokio crate M-io Module: tokio/io labels Oct 29, 2024
Comment on lines +234 to +236
// The bsd module can't be build on Windows, so we completely ignore it, even
// when building documentation.
#[cfg(unix)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if we should abstract this into a macro that better describes what we are doing? Something like cfg_ignore_in_windows_docs!?

@jofas
Copy link
Contributor Author

jofas commented Nov 4, 2024

With these changes, the windows docs can be build with -C docsrs when cross-compiled in the crates-build-env container. There is still a bunch of warnings about broken links in the docs though, like this one, for example. Should I fix those as well? If yes, what would be the best approach here? I probably would just configure out the parts of the docs that don't build on Windows. The main downside of this would be that I make the source of the docs less readable when I replace the nice /// ... lines with cfg_attr!(unix, doc = ["..."]), or such.

@Darksonn
Copy link
Contributor

Darksonn commented Nov 4, 2024

Let's not try to fix the broken links. Can we silence the warnings?

@jofas
Copy link
Contributor Author

jofas commented Nov 4, 2024

We could add #![cfg_attr(windows, allow(rustdoc::broken_intra_doc_links)] to silence the broken link warnings.

@Darksonn
Copy link
Contributor

Darksonn commented Nov 4, 2024

Works for me.

@jofas
Copy link
Contributor Author

jofas commented Nov 7, 2024

This should allow documentation to be buildable on Windows when --cfg docsrs is enabled, but I don't like it. The #[cfg(docsrs)]-related conditional compilation feels a bit fragmented to me as is and this PR as it currently stands makes matters worse IMO. Especially after I found the doc module from #3760, where someone already started mocking Windows-specific items that are part of the public interface. If we were to mock all OS-specific types used throughout the library into such a module (not unlike the loom module), I think we could streamline conditional compilation and the quality of the documentation generated on Windows quite a bit. Would that be something the maintainers would be interested in?

On a side note, I'm not sure why the changes I made to the docs job of the CI workflow result in a Expected — Waiting for status to be reported docs check, especially because the "all systems go" and "basic checks" jobs succeeded.

@jofas jofas marked this pull request as ready for review November 7, 2024 14:24
@mox692
Copy link
Member

mox692 commented Nov 12, 2024

To me, current changes looks fine.

On a side note, I'm not sure why the changes I made to the docs job of the CI workflow result in a Expected — Waiting for status to be reported docs check, especially because the "all systems go" and "basic checks" jobs succeeded.

Since this PR update the docs workflow, I guess the repository's branch protection settings is relevant here?
@Darksonn
Can you confirm this if you have time?

@Darksonn
Copy link
Contributor

I've updated the branch protection rules.

Regarding improving docs support on Windows ... I guess we could. But we would need to repeat all the hacks we use to make windows docs show up on Linux.

@mox692 mox692 merged commit 772e0ca into tokio-rs:master Nov 13, 2024
82 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate M-io Module: tokio/io T-docs Topic: documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to use the docsrs configuration under Windows
3 participants