-
Notifications
You must be signed in to change notification settings - Fork 198
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
Fix missing jQuery error in docs #1321
Conversation
9895768
to
3c3dfb6
Compare
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.
Thanks Jake! 🙏
Is this the source of the issue? readthedocs/sphinx_rtd_theme#1452 |
Yep @bdice, seems like it |
The suggested fix here works but not without throwing a new error |
Ok. FYI @vyasr if you migrate RMM's C++ docs to use Sphinx/Breathe, we might also want to update the Sphinx theme to match the same one used for cudf/cuml/etc. |
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'd like to confirm locally that the suggested workaround isn't sufficient before we merge this.
It'd be better if we didn't have to introduce additional dependencies.
I was able to confirm that this workaround resolves the issue locally without causing other problems. Unfortunately this isn't something we'll be able to confirm in CI, as it appears that query parameters break our preview environments (a separate issue that we'll have to look into): https://downloads.rapids.ai/ci/rmm/pull-request/1321/3c3dfb6/docs/rmm/html/search/?q=allocator&check_keywords=yes&area=default# However I think we should be fine with dropping the extra dependency and using the workaround that they've suggested. |
@ajschmidt8 I'm a bit unsure exactly which piece of that thread helped you. That thread has a few things, like setting |
This reverts commit 3c3dfb6.
I quoted the solution that worked for me. It was this one, which Jake has just added to his PR: readthedocs/sphinx_rtd_theme#1452 (comment) |
before we merge, I will just pull the docs from S3 and check the fix locally so that we can be certain it's working as expected. we'll look into that query parameter issue that I mentioned another time |
confirmed. merging |
/merge |
ah, this incident is probably preventing the I'll just squash merge manually for now. |
The latest versions
4.1
and4.0
ofsphinxcontrib-jquery
have an issue with jQuery installed currently in sphinx. This is causing the search functionality to fail so I've pinned the version to3.0.0
, this resolves the issue.