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

Add inline tabs on Setup and Building page for commands on different systems #1226

Merged
merged 8 commits into from
Nov 23, 2023

Conversation

lancegoyke
Copy link
Contributor

@lancegoyke lancegoyke commented Nov 18, 2023

This PR should close a single TODO in #1196.

This page is a little more complicated than the previous others.

Compile and build has a long list of instructions that are the same for Unix and macOS, so I chose to group them under a single tab rather than duplicate them.

Install dependencies directly refers to "Linux" which is different than the "Unix" we've been using in the other pages. I decided to change the tab to a consistent "Unix" but left the rest of the section unchanged.


📚 Documentation preview 📚: https://cpython-devguide--1226.org.readthedocs.build/getting-started/setup-building/

@lancegoyke
Copy link
Contributor Author

I am open to input about what to do about this build error. It seems the .. tab:: is changing how Sphinx parses the file, but maybe I'm missing something more fundamental.

@hugovk
Copy link
Member

hugovk commented Nov 18, 2023

The error:

/home/runner/work/devguide/devguide/getting-started/setup-building.rst:349: WARNING: Failed to create a cross reference. A title or caption not found: 'macos'
/home/runner/work/devguide/devguide/index.rst:58: WARNING: Failed to create a cross reference. A title or caption not found: 'macos'

It can't find the macos reference, I guess it doesn't work when indented in a tab, so we should try moving these outside and just above the tab group?

   .. _macOS and OS X:
   .. _macOS:

@lancegoyke
Copy link
Contributor Author

It can't find the macos reference, I guess it doesn't work when indented in a tab, so we should try moving these outside and just above the tab group?

   .. _macOS and OS X:
   .. _macOS:

Okay, that was my first instinct, but did not work on my local machine. It still can't find both macos references.

Also peculiar: the tab for Linux has a deps-on-linux reference indented inside the tab which seems to work fine? It looks like this:

.. in the "Install dependencies" section
.. tab:: Unix

   .. _deps-on-linux:

   Linux
   -----

One thing that did seem to work is specifying the reference label name (Sphinx docs). The following code removes one of the "Failed to create a cross reference" warnings:

.. in the "Install dependencies" section
on :ref:`Linux <deps-on-linux>` and :ref:`macOS <macOS>`.  On Windows,

This would require changing the one other reference in devguide/index.rst:58 as well.

Should I try that or do you have another idea?

@hugovk
Copy link
Member

hugovk commented Nov 18, 2023

We also have this earlier in the logs:

/home/runner/work/devguide/devguide/getting-started/setup-building.rst:151: CRITICAL: Unexpected section title.

Unix
----
reading sources... [100%] versions
/home/runner/work/devguide/devguide/getting-started/setup-building.rst:224: CRITICAL: Unexpected section title.

Clang
'''''
/home/runner/work/devguide/devguide/getting-started/setup-building.rst:243: CRITICAL: Unexpected section title.

Optimization
''''''''''''
/home/runner/work/devguide/devguide/getting-started/setup-building.rst:267: CRITICAL: Unexpected section title.

Windows
-------
/home/runner/work/devguide/devguide/getting-started/setup-building.rst:358: CRITICAL: Unexpected section title.

Linux
-----
/home/runner/work/devguide/devguide/getting-started/setup-building.rst:417: CRITICAL: Unexpected section title.

macOS
-----

Perhaps sphinx_inline_tabs doesn't like any headers inside tabs?

@lancegoyke
Copy link
Contributor Author

Perhaps sphinx_inline_tabs doesn't like any headers inside tabs?

I did notice those section titles were not rendering in the HTML, either.

I'm thinking this page cannot make use of the inline tabs as they are currently. The compile, build, and dependency installation processes are all so different that they don't have the same steps. I feel like this page works better as a traditional linear read with section headers.

This is a make command in the "Regenerate configure" section that could use some tabs.

Let me know how you feel about this.

@hugovk
Copy link
Member

hugovk commented Nov 19, 2023

This is a make command in the "Regenerate configure" section that could use some tabs.

I think this only applies to Linux/macOS, there's no make.bat in the CPython repo.


How about this?

Add Linux and macOS tabs to the "Install dependencies" section. We can move all the references just above the header:

.. _build-dependencies:
.. _deps-on-linux:
.. _macOS and OS X:
.. _macOS:

Install dependencies
====================

This section explains how to install additional extensions (e.g. ``zlib``)
on :ref:`Linux <deps-on-linux>` and :ref:`macOS`.  On Windows,
extensions are already included and built automatically.

.. tab:: Linux

[...]

.. tab:: macOS

[...]

And that can be enough for this page, for now. We can consider followups later.

@lancegoyke
Copy link
Contributor Author

I think this only applies to Linux/macOS, there's no make.bat in the CPython repo.

Ahhhh, okay, I think I got mixed up because I saw this one, but it's in the Doc folder.


I think that's a good way forward. I just pushed two commits and my section title errors have disappeared locally.

Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

Please also add the JS snippet at the top ;)

getting-started/setup-building.rst Outdated Show resolved Hide resolved
@lancegoyke
Copy link
Contributor Author

I think that was everything! I can't believe I missed the JavaScript, lollollol

Copy link
Member

@hugovk hugovk 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!

@hugovk hugovk merged commit 6d96e57 into python:main Nov 23, 2023
4 checks passed
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.

2 participants