-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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: implementation of build.commands
#9150
Conversation
Neat how little code this is. Definitely needs a lot more done around the edges to make it nicer, but great that we already have all the basic infrastructure to make it work. 👍 |
41451f8
to
13b52de
Compare
Minimal implementation of `build.commands` as a POC to show how it could work. It's using a `.readthedocs.yaml` similar to this one: ```yaml version: 2 build: os: ubuntu-20.04 commands: - mkdir output/ - echo "Hello world" > output/index.html tools: python: "3" ``` This, of course, is not a good implementation and it's done pretty quick as a way to show what parts of the code are required to be touched. I think this will help with the discussion about how the UX around `build.commands` could work. It defines an "implicit contract" where the `output/` folder under the repository checkout will be uploaded to S3 and no HTML integrations will be done.
The method `install_build_tools` is not tied to the Python environment, and it's more related to the build's director, instead. I'm moving it to the `BuildDirector` class also because it's needed there when using `build.commands` as the first step to execute to install the `build.tools` specified by the user to build their docs.
13b52de
to
2286206
Compare
I'm asking review for the three of you @ericholscher, @agjohnson, and @stsewd because I think this is an important change that will mold our path moving forward regarding the UX. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks close to me. I think we're close enough to get it out next week for testing. There's lots of additional work to be done in docs, etc. -- but this is enough for now.
@@ -473,7 +473,7 @@ Specify a list of commands that Read the Docs will run on the build process. | |||
When ``build.commands`` is used, none of the :term:`pre-defined build jobs` will be executed. | |||
(see :doc:`/build-customization` for more details). | |||
This allows you to run custom commands and control the build process completely. | |||
The ``output/`` directory (relative to the checkout's path) will be uploaded and hosted by Read the Docs. | |||
The ``_readthedocs/html`` directory (relative to the checkout's path) will be uploaded and hosted by Read the Docs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still feels really small compared to how important the information is, but we can improve it over time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. This has to be a lot more prominent. However,
- I wasn't sure how to make it fit here in a good way
- I'm not sure this is the right place for explaining this more. From the config file, we should point users to the "Build customization" page and explain all the things in detail there. Otherwise, we will be overloading the config file reference
I'm thinking we may need a section in the "Build customization" page that list all the considerations required to use build.commands
or something like that we can link from here. I feel I'm not coming with a good idea of how to write this, tho 🤷🏼
Co-authored-by: Eric Holscher <[email protected]>
Tests were failing because it's not possible to use `self.build` at this point.
Execute `asdf python reshim` automatically when we detect that pip/mamba/conda/poetry were called to install a Python package that may install an executable.
Make sure that `config.build.commands` is not empty.
When updating the `Version` object via the API on success build, we were using the same value we received from the API for the `Version` object. This commit uses the `doctype` value from the config file used to build the version. This way, we keep the `Version.documentation_type` updated when there are changes in the config file for a particular version.
I updated the PR with all the suggestions that I received and improved the code a little bit. Now, it handles reshiming automatically when it detects a Python package manager installing something and it also uses a For now, we are using template-based notifications but we plan to change that because none of us like this pattern. However, it's good enough, for now, to move forward and don't block this feature on that.
I'm happy to merge this during this week and deploy it the next one 👍🏼 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great. 👍 from me with the last bits of new feedback resolved
This will allow us to be smarter about _when to reshim_. With this update, it will be handled automatically by `asdf` and we won't require a custom chunk of code in our application: https://github.com/readthedocs/readthedocs.org/blob/a5965129c61b9bcdff2f2098ff7ce7f8c093dc74/readthedocs/doc_builder/director.py#L373-L386 Currently, multi-lines commands that install something with `pip` and immediate after that, inside the multi-line command try to use the executable installed, fail because it's not automatically reshimed. By reshiming at `asdf` level, this case will be solved. Related: readthedocs/readthedocs.org#9150 (comment) Related: asdf-community/asdf-python#136
* asdf: update `asdf` and all its plugins This will allow us to be smarter about _when to reshim_. With this update, it will be handled automatically by `asdf` and we won't require a custom chunk of code in our application: https://github.com/readthedocs/readthedocs.org/blob/a5965129c61b9bcdff2f2098ff7ce7f8c093dc74/readthedocs/doc_builder/director.py#L373-L386 Currently, multi-lines commands that install something with `pip` and immediate after that, inside the multi-line command try to use the executable installed, fail because it's not automatically reshimed. By reshiming at `asdf` level, this case will be solved. Related: readthedocs/readthedocs.org#9150 (comment) Related: asdf-community/asdf-python#136 * Docs: update readme to mention `ubuntu-22.04` and `buildx` * Tests: update version to check
Initial implementation of
build.commands
as a beta feature to expose to user and collect more use cases.Build without using any tool
(https://github.com/readthedocs/test-builds/blob/build-commands/.readthedocs.yaml)
Build details page
index.html
file served by El ProxitoBuild using
pelican
(https://github.com/readthedocs/test-builds/blob/pelican/.readthedocs.yaml)
Build details page
index.html
file served by El ProxitoDecisions / Notes
build.commands
on the config fileoutput/
folder under the repository checkout will be uploaded to S3build.os
andbuild.tools
are mandatory when usingbuild.commands
build.tools
not required since it's possible to generate HTML without any of these languages as shown in the first example where onlyecho
command is usedasdf
requires re-shim the binaries after installing new CLI tools with Python to be able to find them when executing any of these commands. There was an attempt to re-shim automatically at Automatically run reshim? asdf-community/asdf-python#37 but they removed thepost_<tool>_<shim>
because Revert to using exec when running a shim asdf-vm/asdf#502. This may need some extra research to make the UX better. However, for now, I'm addingasdf reshim
to mybuild.commands
. We could executeasdf reshim python
after each of thebuild.commands
, for now as wellI didn't update the "Build customization" page, but I'm happy to add a small example there (https://docs.readthedocs.io/en/stable/build-customization.html) if we consider it's good at this point. However, it may require a good amount of re-writing on that page to make the distinction between the two approaches clearRelated #9062 and others linked into that issue.