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

Sphinx 7 support #744

Merged
merged 11 commits into from
Oct 29, 2023
Merged

Sphinx 7 support #744

merged 11 commits into from
Oct 29, 2023

Conversation

LecrisUT
Copy link
Contributor

@LecrisUT LecrisUT commented Jun 17, 2023

Cannot install testing suite because of dependency hell. Any advice on this? See my fork PR LecrisUT#1. Any ways to drop myst-nb and sphinx_thebe testing requirement? It is hell to try and detangle their testing requirements.

@welcome
Copy link

welcome bot commented Jun 17, 2023

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out EBP's Code of Conduct and our Contributing Guide, as this will greatly help the review process.

Welcome to the EBP community! 🎉

@LecrisUT LecrisUT force-pushed the sphinx-7 branch 2 times, most recently from c94bf43 to 9313f58 Compare June 17, 2023 23:52
@PhilipVinc
Copy link

myst-nb actually supports sphinx 7, we simply need the maintainers to tag a new release...

@LecrisUT
Copy link
Contributor Author

LecrisUT commented Jul 6, 2023

Indeed, you can see the author there :D executablebooks/MyST-NB#524. Right now Sphinx-thebe is the hold-up executablebooks/sphinx-thebe#67. I don't know how to make a good testing suite there to decouple it from sphinx-book-theme which otherwise creates a cyclic dependency issue.

@Eutropios
Copy link

Is there any word on this being resolved?

@LecrisUT
Copy link
Contributor Author

If we can yolo both this and sphinx-thebe to get rid of the silly cyclic dependency the CIs will then be back running properly and we can pick up from there. @choldgraf any thoughts?

@choldgraf
Copy link
Member

choldgraf commented Oct 29, 2023

I've just made a release of sphinx-thebe to unblock this on the version pinning issue:

https://github.com/executablebooks/sphinx-thebe/releases/tag/v0.3.0

Myst NB also has a release candidate out (1.0.0rc0) that removes the sphinx pinning issue there too

Sorry for the slowness - I or @agoose77 can try to help provide review to get this over the finish line if @LecrisUT has cycles to get the tests passing

@LecrisUT LecrisUT closed this Oct 29, 2023
@LecrisUT LecrisUT reopened this Oct 29, 2023
@LecrisUT
Copy link
Contributor Author

Ok, I'll get the CIs back and running, and mark some of them as experimental if needed, then we can open new PRs to de-mark the experimental. Sounds good?

Btw, what are your thoughts on making some of the CIs more templetize (or even importable) to ve consistent across each other. I can write a mock-up over here and then have them moved to a new repo maintained more regularly under executablebooks?

@choldgraf
Copy link
Member

For now I'd recommend just taking the minimal actions and complexity needed to support the latest Sphinx and be reasonably certain we aren't introducing obvious regressions. Improving broader infrastructure sounds good but like it might add a lot of extra work to getting this unblocked. Right now we are extremely heavily limited in maintainer capacity...

@LecrisUT
Copy link
Contributor Author

LecrisUT commented Oct 29, 2023

@choldgraf Ok, for now here is some minimal updates, with a few fixed pytests. The currently failing ones seem trivial, just need to update the regression test files for different versions.

The windows CI does not work in how I wrote it. Can you help with that? I will mark it experimental for the time-being

Sorry a bit slow to push commits because pre-commit is having trouble updating on my side

@LecrisUT
Copy link
Contributor Author

All ready on this PR, might add some CI improvements in another PR. The only one I don't understand is the windows one :(. Should be fine for at least a pre-release tag, since myst-nb is also gated as pre-release?

Copy link
Member

@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.

Looks good to me - let's merge it in and iterate on PRs from there. I'm not sure what's up with the Windows bug either, but I don't think it's related to this PR. Thanks for making this effort!

@choldgraf choldgraf merged commit 3dd7770 into executablebooks:master Oct 29, 2023
15 of 17 checks passed
@welcome
Copy link

welcome bot commented Oct 29, 2023

Congrats on your first merged pull request in this project! 🎉
congrats

Thank you for contributing, we are very proud of you! ❤️

@Eutropios
Copy link

Thank you both for the hard work! @LecrisUT @choldgraf

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.

4 participants