Skip to content

Faster makefile for doc development offline #6623

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

Merged

Conversation

Pierre-Sassoulas
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas commented May 16, 2022

Type of Changes

Type
✨ New feature
🔨 Refactoring

Description

I did a lot of doc modification recently for #6589 and the makefile installing dependency each time is taking a lot of time unnecessarily. I'm also thinking about lazy generating message files etc.

@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.14.0 milestone May 16, 2022
@Pierre-Sassoulas Pierre-Sassoulas marked this pull request as draft May 16, 2022 07:46
@coveralls
Copy link

coveralls commented May 16, 2022

Pull Request Test Coverage Report for Build 2331129273

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 95.363%

Totals Coverage Status
Change from base Build 2328584656: 0.0%
Covered Lines: 16060
Relevant Lines: 16841

💛 - Coveralls

@Pierre-Sassoulas Pierre-Sassoulas force-pushed the faster-makefile-for-doc branch from 2f1d6e1 to 2ea610a Compare May 16, 2022 08:49
@@ -13,7 +13,7 @@ PYTHONPATH =
# Internal variables.
PAPEROPT_a4 = -D latex_paper_size=a4
PAPEROPT_letter = -D latex_paper_size=letter
ALLSPHINXOPTS = -d $(BUILDDIR)/doctrees -T -E -W --keep-going $(PAPEROPT_$(PAPER)) $(SPHINXOPTS) .
ALLSPHINXOPTS = -d $(BUILDDIR)/doctrees -T -W --keep-going $(PAPEROPT_$(PAPER)) $(SPHINXOPTS) .
Copy link
Member Author

Choose a reason for hiding this comment

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

-E don't use a saved environment, always read all files

In the CI we have a fresh environment, so this option is just making repeated build slower locally

@Pierre-Sassoulas Pierre-Sassoulas marked this pull request as ready for review May 16, 2022 08:52
doc/Makefile Outdated

html: build-html
@echo "Install dependencies"
$(PIP) install -r requirements.txt
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the dependencies should be installed before calling build-html right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in latest commit.

@@ -87,6 +87,10 @@ Tips for Getting Started with Pylint Development
Building the documentation
----------------------------

We use **tox** for building the documentation::
You can use the makefile and build-html command for building the documentation during developement::
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's suggest to run make html if you want to build the documentation and use build-html if you want to quickly test a small change. I think it's good to have make html be the standard as we could other checks next to linkcheck that would then be run automatically locally like a "documentation pre-commit".

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

@@ -1,4 +1,3 @@
Sphinx==4.5.0
sphinx-reredirects<1
furo==2022.4.7
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need the -e . for RTD as it needs access to pylint. We provide doc/requirements.tx while we're cd'ed into root.

I know I looked into this previously but couldn't figure out an elegant solution of allowing both RTD and cd docs + pip install -r ... to work.

Copy link
Member Author

Choose a reason for hiding this comment

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

How about we suggest to install from the root with doc/requirements.txt, and we remove the requirement from the makefile ? (I.e. it's the job of the prepare-base job to install the proper dependencies.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we cd in a makefile? That way we could cd before and after installing the documentation dependencies.

I do think it would be nice to have one command you can run to completely make the documentation without any further trouble.

Copy link
Member Author

@Pierre-Sassoulas Pierre-Sassoulas May 16, 2022

Choose a reason for hiding this comment

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

@Pierre-Sassoulas Pierre-Sassoulas force-pushed the faster-makefile-for-doc branch from fe965d2 to 21f099a Compare May 16, 2022 09:51
@Pierre-Sassoulas Pierre-Sassoulas force-pushed the faster-makefile-for-doc branch from 21f099a to b94d9ac Compare May 16, 2022 09:53
Copy link
Collaborator

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

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

One final nitpick. If this passes RTD this LGTM!

@Pierre-Sassoulas Pierre-Sassoulas force-pushed the faster-makefile-for-doc branch from 89bb089 to 95ed95f Compare May 16, 2022 09:58
@Pierre-Sassoulas Pierre-Sassoulas merged commit 95be3b9 into pylint-dev:main May 16, 2022
@Pierre-Sassoulas Pierre-Sassoulas deleted the faster-makefile-for-doc branch May 16, 2022 10:21
@Pierre-Sassoulas Pierre-Sassoulas removed this from the 2.14.0 milestone May 17, 2022
@Pierre-Sassoulas Pierre-Sassoulas added the Maintenance Discussion or action around maintaining pylint or the dev workflow label Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation 📗 Enhancement ✨ Improvement to a component Maintenance Discussion or action around maintaining pylint or the dev workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants