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

📖 Add ReadTheDocs automation #982

Merged
merged 1 commit into from
Apr 17, 2024
Merged

Conversation

LecrisUT
Copy link
Contributor

@LecrisUT LecrisUT commented Mar 14, 2024

I will create a mystmd-temp slug to test if it all works as expected

Closes #968

@LecrisUT LecrisUT force-pushed the ci/rtd branch 3 times, most recently from e602eb5 to d8c2cc4 Compare March 14, 2024 11:19
.readthedocs.yaml Outdated Show resolved Hide resolved
@LecrisUT LecrisUT force-pushed the ci/rtd branch 5 times, most recently from 26a540e to 02c8806 Compare March 14, 2024 13:15
@LecrisUT LecrisUT marked this pull request as ready for review March 14, 2024 13:15
@LecrisUT
Copy link
Contributor Author

LecrisUT commented Mar 14, 2024

Any ideas on what is going on here? Why is packages/myst-parser/docs being built, and why does it not seem to be doing that when I run manually?

Edit: It seems RTD runs all command lines from the same working directory 😮‍💨. Have to chain a cd docs && to work around that

@LecrisUT LecrisUT force-pushed the ci/rtd branch 5 times, most recently from 1e07c14 to 1ef8cfa Compare March 14, 2024 14:08
@LecrisUT
Copy link
Contributor Author

LecrisUT commented Mar 14, 2024

Ready for review. Everything seems to be working: https://beta.readthedocs.org/projects/mystmd-temp/builds/?version=1

There is one issue though, the paths and url are not relocatable, i.e. the index root is set to https://mystmd-temp--1.org.readthedocs.build/en/1/ (/{lang}/{version}), but the generated documents are searching for /_assets, etc. (the files are accessible under /{lang}/{version}/_assets). It would be nice if this pathing can be fixed, since it seems that we have version control by default thanks to RTD's flyout.

For the sake of this PR, we can serve a single-version page.

Navigation also seems slightly broken in that https://mystmd-temp--1.org.readthedocs.build/quickstart does not translate to https://mystmd-temp--1.org.readthedocs.build/quickstart.html. Adding symlinks did not seem to work

@LecrisUT LecrisUT force-pushed the ci/rtd branch 2 times, most recently from 4e7b250 to af8b767 Compare March 14, 2024 15:00
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
@choldgraf
Copy link
Collaborator

This looks great to me - I believe that in RTD you can remove the en/latest prefix by checking this option (on the old RTD interface, it looks like you're using a new one):

CleanShot 2024-03-14 at 10 29 52@2x

@LecrisUT
Copy link
Contributor Author

This looks great to me - I believe that in RTD you can remove the en/latest prefix by checking this option (on the old RTD interface, it looks like you're using a new one):

Indeed I have tried that, it did solve the /{lang}/{version}/ pathing, but the page redirection issue was still persistent. Maybe it's an issue on RTD for that that one.

But I figured that setting BASE_URL=${READTHEDOCS_CANONICAL_URL} is also compatible here, and it keeps the multi-version support that we get automatically from RTD 😉

@LecrisUT
Copy link
Contributor Author

Btw, also worth investigating the console errors reported. I am not sure if it relates to the path sanitization, but they all seem to point to the build/_shared script paths. A bit hard to navigate though because there is no source-map to the .ts fiels.

Screenshot_20240318_110444

Copy link
Collaborator

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

I suggest that we merge this one in as-is, and then continue iteration on RTD preview builds via a RTD project that's linked to this repository. Then we can consider this "done" when we get a PR preview of our documentation.

@rowanc1 does that make sense to you?

@LecrisUT
Copy link
Contributor Author

In that case when creating the RTD settings, the main website builder should be disabled so that myst.readthedocs.io is not yet created and/or the SEO does not prefer to the broken page yet.

@rowanc1 rowanc1 changed the title ci: Add ReadTheDocs automation 📖 Add ReadTheDocs automation Apr 17, 2024
@rowanc1 rowanc1 merged commit a200549 into jupyter-book:main Apr 17, 2024
8 checks passed
@rowanc1
Copy link
Collaborator

rowanc1 commented Apr 17, 2024

@choldgraf and I are going to do some testing now in the mystmd RTD project.

- npm run build
- npm run --workspace packages/mystmd link
- myst --version
- cd docs && BASE_URL=${READTHEDOCS_CANONICAL_URL} myst build --html
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was planning on quickly dropping the BASE_URL part here, but anyway, there are other issues to resolve with this, so can do that at that time. Also drop the bottom comment as unplanned upstream anyway

@rowanc1
Copy link
Collaborator

rowanc1 commented Apr 17, 2024

The preview is up here:
https://mystmd.readthedocs.io/en/latest/

The biggest issue (beyond hydration) is around the .html requirement.

@LecrisUT
Copy link
Contributor Author

Yeah, both issues are on myst-to-react afaiu. At least the .html should be straighforward to pin down and it would make the RTD generated sites usable. RTD could also potentially fix it with: readthedocs/readthedocs.org#11220, but in the current configuration it is not possible.

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.

Developer suggestion: Build a static HTML preview of the documentation in our CI/CD
3 participants