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

Use make in build sphinx docs #58

Merged
merged 6 commits into from
Jul 30, 2024
Merged

Use make in build sphinx docs #58

merged 6 commits into from
Jul 30, 2024

Conversation

lochhh
Copy link
Contributor

@lochhh lochhh commented Jul 10, 2024

Description

What is this PR

  • Bug fix
  • Addition of a new feature
  • Other

Why is this PR needed?
This PR adds the optional use-make input to the build_sphinx_docs action to allow docs to be built using the make utility, such that we can use, e.g.:

  • make html instead of sphinx-build source build
  • make clean html instead of rm -rf build && sphinx-build source build
  • make linkcheck instead of sphinx-build source build -b linkcheck

This will also allow custom build targets in the make file, e.g., to automate the API index page generation.

Since make places the built html pages in docs/build/html/, whereas the default sphinx-build places these in docs/build (as pointed out by @niksirbi), the optional use-make input is also added to the deploy_sphinx_docs action to help identify the location of the built documentation for deployment.

To ensure all current repositories using these actions remain unaffected, use-make is set to false by default, i.e. they will continue to use sphinx-build and project maintainers can opt-in as needed.

How has this PR been tested?

This has been tested

Does this PR require an update to the documentation?

Updated description in all README.mds

Checklist:

  • The code has been tested locally
  • Tests have been added to cover all new functionality
  • The documentation has been updated to reflect any changes
  • The code has been formatted with pre-commit

@lochhh lochhh requested a review from a team July 10, 2024 08:51
@adamltyson
Copy link
Member

Just for my own curiosity:
a) Why do we need/want this
b) Would this have any affect upon local workflows (we have lots of website built using this action now)

@niksirbi
Copy link
Member

My understanding is that this has two benefits:

  • neater syntax
  • the ability to add custom make commands on a per-project basis. In the example linked above, @lochhh added a custom build step in movement that auto-generates the entire API index page.

But I agree that it's important to make sure that this doesn't break projects with the "default" Makefile. Afaik it shouldn't, but maybe we can test that via one such project (e.g. HowTo, NeuroBlueprint), after merging this to main and before we make a new actions release?

@lochhh
Copy link
Contributor Author

lochhh commented Jul 10, 2024

This will work with the default Makefile (created by sphinx-quickstart) - which I believe our projects use?

@niksirbi
Copy link
Member

This will work with the default Makefile (created by sphinx-quickstart) - which I believe our projects use?

Yeah, that should be the case. I created the original Makefile with quickstart (including the one in the cookiecutter), and I think all other projects just copied that.

@niksirbi
Copy link
Member

niksirbi commented Jul 10, 2024

Btw I just noticed that if you use sphinx-build the index.html is located in build/index.html, whereas when using make html it's in docs/html/index.html. Will that affect the rendering of the webpage by gh-pages? Maybe the action we use during deployment is robust to that?

@lochhh
Copy link
Contributor Author

lochhh commented Jul 11, 2024

Btw I just noticed that if you use sphinx-build the index.html is located in build/index.html, whereas when using make html it's in docs/html/index.html. Will that affect the rendering of the webpage by gh-pages? Maybe the action we use during deployment is robust to that?

You're right, it will be in docs/build/html/index.html and we will need to change publish_dir to docs/build/html 🤔

@adamltyson
Copy link
Member

Before merging can we come up with a plan to make sure every website is going to work fine following these changes. I assume it will all be ok, but just in case, the ones that I know of that use this action are:

@niksirbi
Copy link
Member

@niksirbi
Copy link
Member

niksirbi commented Jul 11, 2024

A potential plan:

  • Make these changes "optional" via an argument that's false by default, something like the following in both build and publish actions:
  use-make:
    description: 'Build sphinx docs with `make'
    required: false
    type: boolean
    default: false
  • Merge and release actions
  • Enable this boolean in the workflows for movement and maybe some other low-stakes websites, like HowTo
  • After a while, once we're sure it works, for everyone, remove that optional boolean?
  • Alternatively we can leave the boolean in and leave the choice up to each website's maintainer

@lochhh lochhh requested a review from niksirbi July 29, 2024 16:58
@lochhh
Copy link
Contributor Author

lochhh commented Jul 29, 2024

A potential plan:

  • Make these changes "optional" via an argument that's false by default, something like the following in both build and publish actions:
  use-make:
    description: 'Build sphinx docs with `make'
    required: false
    type: boolean
    default: false
  • Merge and release actions
  • Enable this boolean in the workflows for movement and maybe some other low-stakes websites, like HowTo
  • After a while, once we're sure it works, for everyone, remove that optional boolean?
  • Alternatively we can leave the boolean in and leave the choice up to each website's maintainer

I've made the changes as suggested and forked the movement repo to test the docs build and deploy.

@niksirbi
Copy link
Member

I've made the changes as suggested and forked the movement repo to test the docs build and deploy.

Seems to be working fine, based on the rendered website from the fork.
Time to merge and make a new actions release?

Copy link
Member

@niksirbi niksirbi left a comment

Choose a reason for hiding this comment

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

Looks good to me now, and I like how you've documented this in the READMEs @lochhh

@niksirbi niksirbi merged commit 5102341 into main Jul 30, 2024
@niksirbi niksirbi deleted the make-docs branch July 30, 2024 07:54
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.

3 participants