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

developer contributing docs - llvm, sqlite3, sphinx-rtd-theme, and make html #7367

Closed
wants to merge 10 commits into from

Conversation

sgbaird
Copy link
Contributor

@sgbaird sgbaird commented Sep 4, 2021

Here's a summary:

  • There seems to be an issue in the docs related to the channel used for llvm
  • There seems to be a Windows-specific bug where conda can't find sqlite3.dll when trying to commit using the pre-commit hooks, so I added some instructions for overcoming that issue.
  • The docs can't build because it sphinx-rtd-theme isn't installed, so I added that to the pip install
  • I added instructions that make html is run under docs (should be obvious to anyone familiar with make, seeing as there is no MAKEFILE under docs/source, but saves someone the extra step)

Reference an existing issue

#7362 #7354

@sgbaird sgbaird changed the title Dev docs developer contributing docs - llvm, sqlite3, sphinx-rtd-theme, and make html Sep 4, 2021
Copy link
Member

@esc esc left a comment

Choose a reason for hiding this comment

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

Looking good so far, some open questions on the correct sphinx extensions to install. Thank you for working on this. 🙏

To use these pre-commit hooks on Windows, it may be necessary to copy
``sqlite3.dll`` from ``C:\Users\<USER>\anaconda3\Library\bin`` to
``C:\Users\<USER>\anaconda3\DLLs`` or modify your PATH (`source <https://stackoverflow.com/a/55642416/13697228>`_).

Copy link
Member

Choose a reason for hiding this comment

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

I can't verify this due to lack of a developmental machine, is there anyone who can confirm this work?

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'm assuming anyone means core devs, but for reference, this was necessary for me to do on Windows to get pre-commit hooks working after following the contrib directions as closely as possible.

Copy link
Member

Choose a reason for hiding this comment

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

Well, yes, core devs (on a loose definition of the term), would be ideal. But I think we would also be satisfied with anyone other than the author confirming this. Thank you!

Copy link
Member

Choose a reason for hiding this comment

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

I can't replicate the issue on Windows - this seems like a little bit of an odd situation to have got into, and I'd suggest we shouldn't include the note on the basis that the linked source doesn't provide enough background about what's going on here for it to be clear that this is generally good advice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, maybe something weird on my end, then. Sounds good!

Copy link
Member

Choose a reason for hiding this comment

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

Are you planning to remove this note?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, yes, forgot about this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pre-commit hooks in a conda environment on Windows have still been a huge pain. Unfortunately, my workaround has just been to uninstall the hooks in many cases.


$ pip install sphinx_bootstrap_theme
$ pip install sphinx_bootstrap_theme sphinx-rtd-theme
Copy link
Member

Choose a reason for hiding this comment

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

Two comments here. First, I thought this was mis-spelling, since my personal makefile contains sphinx_rtd_theme. But then I checked on anaconda.org and both sphinx_rtd_theme AND sphinx-rtd-theme exist.

Screen Shot 2021-09-06 at 13 13 07

Screen Shot 2021-09-06 at 13 12 43

Judging from downloads, I would tend to argue that sphinx_rtd_theme is probably what we want?

Also, I am not sure we actually need sphinx_bootstrap_theme. I do not have it in my environment:

Screen Shot 2021-09-06 at 13 19 34

And it builds fine. IIRC I think we switched from the bootstrap to the rtd theme some time ago but the docs were not updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interestingly, when I read this, I even though it was a mispelling on my part, but when I went to https://github.com/readthedocs/sphinx_rtd_theme it turns out their instructions state pip install sphinx-rtd-theme. Probably fine to go with just sphinx_rtd_theme and leave out sphinx_bootstrap_theme since it doesn't seem to have been touched since 2019.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I though this was conda not pip. Are we asking people to pip install into a conda environment? Do we want to recommend conda instead, since the packages are available there?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, probably change this to conda install.

Copy link
Member

@esc esc left a comment

Choose a reason for hiding this comment

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

Update on installation procedure.


$ pip install sphinx_bootstrap_theme
$ pip install sphinx_bootstrap_theme sphinx-rtd-theme
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, probably change this to conda install.

@sgbaird
Copy link
Contributor Author

sgbaird commented Nov 4, 2021

Changed it to conda install

@esc
Copy link
Member

esc commented Nov 9, 2021

Changed it to conda install

Thank you! Once the reference to sqllite.dll has been removed I think this will be ready to merge!

@sgbaird thank you for your efforts on this front!

@stuartarchibald
Copy link
Contributor

Just checking in on the state of this PR. @sgbaird was wondering if you have time to address the changes above, if not, it's no problem, one of the core developers can adopt the patch as needed. Many thanks.

@@ -415,12 +421,12 @@ It is built with `Sphinx <http://sphinx-doc.org/>`_ and
`numpydoc <https://numpydoc.readthedocs.io/>`_, which are available using
conda or pip; i.e. ``conda install sphinx numpydoc``.

To build the documentation, you need the bootstrap theme::
To build the documentation, you need the bootstrap and rtd themes::
Copy link
Member

Choose a reason for hiding this comment

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

Is this still accurate? RTD doesn't seem to install sphinx_bootstrap_theme: e.g. https://readthedocs.org/api/v2/build/15944491.txt

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 don't think it needs sphinx_bootstrap_theme.

@stuartarchibald stuartarchibald added 4 - Waiting on reviewer Waiting for reviewer to respond to author and removed 4 - Waiting on author Waiting for author to respond to review labels Feb 2, 2022
Copy link
Member

@gmarkall gmarkall left a comment

Choose a reason for hiding this comment

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

Many thanks for the updates - this looks good to me now!

Copy link
Member

@gmarkall gmarkall left a comment

Choose a reason for hiding this comment

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

Whoops, I need to walk back on my previous review - conda install sphinx-rtd-theme does not work, only pip provides a package with the dashes in the name as opposed to underscores.

Co-authored-by: Graham Markall <535640+gmarkall@users.noreply.github.com>
@esc esc mentioned this pull request Jan 27, 2023
@esc
Copy link
Member

esc commented Jan 27, 2023

@gmarkall I have resolved the conflicts on this one and submitted a fresh PR at: #8730 -- would you have some spare time to review it please? Thank you!

@esc
Copy link
Member

esc commented May 4, 2023

#8730 has been marked as ready-to-merge -- closing this one now.

@esc esc closed this May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - Waiting on reviewer Waiting for reviewer to respond to author doc Effort - medium Medium size effort needed
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants