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

Build: don't run default steps if no sphinx or mkdocs #11810

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

humitos
Copy link
Member

@humitos humitos commented Nov 28, 2024

This PR modifies the build to not run default commands when the user doesn't specify sphinx or mkdocs configs in the YAML file.

This allows to use the following YAML file to build with Docusaurus, for example:

version: 2

build:
  os: ubuntu-24.04
  tools:
    nodejs: "22"
  jobs:
    install:
      - cd docs/ && npm install
    build:
      html:
        - cd docs/ && npm run build
    post_build:
      - mkdir --parents $READTHEDOCS_OUTPUT/html/
      - cp --recursive docs/build/* $READTHEDOCS_OUTPUT/html/

This is the structure I've been wanting to have to allow any type of doctool to work in our platform following the build.jobs pattern.

The code deployed in production doesn't allow this because it runs automatically create_environment, install_core_dependencies and install_requirements even if the project is not using Python --which is incorrect. It fails with the generic error: "Unknown problem. There was a problem with Read the Docs while building your documentation"

This PR detects if the user is defining sphinx or mkdocs and if it's not, it doesn't run those jobs and leave the user to handle the build workflow completely.

This PR modifies the build to not run default commands when the user doesn't
specify `sphinx` or `mkdocs` configs in the YAML file.

This allows to use the following YAML file to build with Docusaurus, for
example:

```yaml
version: 2

build:
  os: ubuntu-24.04
  tools:
    nodejs: "22"
  jobs:
    install:
      - cd docs/ && npm install
    build:
      html:
        - cd docs/ && npm run build
    post_build:
      - mkdir --parents $READTHEDOCS_OUTPUT/html/
      - cp --recursive docs/build/* $READTHEDOCS_OUTPUT/html/
```

This is the structure I've been wanting to have to allow any type of doctool to
work in our platform following the `build.jobs` pattern.

The code deployed in production doesn't allow this because it runs automatically
`create_environment`, `install_core_dependencies` and `install_requirements`
even if the project is not using Python --which is incorrect. It fails with the
generic error: " Unknown problem. There was a problem with Read the Docs while
building your documentation"

- https://app.readthedocs.org/projects/test-builds/builds/26425680/
- https://read-the-docs.sentry.io/issues/4458662274/

This PR detects if the user is defining `sphinx` or `mkdocs` and if it's not, it
doesn't run those jobs and leave the user to handle the build workflow
completely.

* Related #11216
* Related #11551
@humitos humitos requested a review from ericholscher November 28, 2024 13:59
@humitos humitos requested a review from a team as a code owner November 28, 2024 13:59
@humitos
Copy link
Member Author

humitos commented Nov 28, 2024

Note this may not be the best code implementation and I'm happy to adapt it. However, I wanted to have something working to show the use case I've been wanting to have in our platform and show a real example.

We are doing plenty of magic in our config file (eg. auto populating fields that the user didn't declare) and this code may need some refactor/adaption to be sure we are not breaking patterns that were working previously.

@ericholscher ericholscher requested a review from stsewd December 2, 2024 17:45
@ericholscher
Copy link
Member

Conceptually this makes sense to me. I'm not 100% sure the best options for implementation, so tagging @stsewd on this as well. I think this is an obvious next step for the build.jobs.build logic, and makes sense to keep trying to make it support more generic logic.

@agjohnson
Copy link
Contributor

Agreed on avoiding these default steps.

Note, there are some projects in the middle here that don't have a sphinx key but are using Sphinx builder. So, maybe we could use a middle step here too, like if there are build jobs and no sphinx key, we don't do the default steps?

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