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

Detect and use system MathJax-3.x #36098

Merged
merged 4 commits into from
Aug 27, 2023
Merged

Conversation

orlitzky
Copy link
Contributor

@orlitzky orlitzky commented Aug 17, 2023

Resolves #30296

@@ -374,7 +374,7 @@ def set_intersphinx_mappings(app, config):
if os.environ.get('SAGE_USE_CDNS', 'no') == 'yes':
mathjax_path = "https://cdn.jsdelivr.net/npm/mathjax@3/es5/tex-chtml.js"
else:
mathjax_path = 'mathjax/tex-chtml.js'
mathjax_path = 'tex-chtml.js'
html_common_static_path += [MATHJAX_DIR]
Copy link
Collaborator

@kwankyu kwankyu Aug 18, 2023

Choose a reason for hiding this comment

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

This change does not break the documentation, but has a problem which is the reason of the prefix mathjax/.

This is the content of the directory _static built by Sphinx (with the PR):

$ cd _static/
$ ls
_sphinx_javascript_frameworks_compat.js mml-svg.js
a11y                                    node-main.js
adaptors                                output
basic.css                               pdf.png
check-solid.svg                         plot_directive.css
clipboard.min.js                        plus.png
copy-button.svg                         pygments.css
copybutton.css                          sageicon.png
copybutton.js                           sagelogo.png
copybutton_funcs.js                     scripts
core.js                                 searchtools.js
custom-furo.css                         skeleton.css
debug.css                               sphinx_highlight.js
doctools.js                             sre
documentation_options.js                startup.js
favicon.ico                             styles
file.png                                tex-chtml-full.js
input                                   tex-chtml.js
jquery-3.6.0.js                         tex-mml-chtml.js
jquery.js                               tex-mml-svg.js
jupyter-sphinx.css                      tex-svg-full.js
language_data.js                        tex-svg.js
latest.js                               thebe-sage.js
loader.js                               thebelab-helper.js
logo_sagemath_black.svg                 thebelab.css
logo_sagemath_white.svg                 ui
minus.png                               underscore-1.13.1.js
mml-chtml.js                            underscore.js

Note that all mathjax files are mixed with other static files while the original (without this PR) _static directory looks like this:

$ cd _static/
$ ls
_sphinx_javascript_frameworks_compat.js logo_sagemath_white.svg
basic.css                               mathjax  <----------------------------------
check-solid.svg                         minus.png
clipboard.min.js                        pdf.png
copy-button.svg                         plot_directive.css
copybutton.css                          plus.png
copybutton.js                           pygments.css
copybutton_funcs.js                     sageicon.png
custom-furo.css                         sagelogo.png
debug.css                               scripts
doctools.js                             searchtools.js
documentation_options.js                skeleton.css
favicon.ico                             sphinx_highlight.js
file.png                                styles
jquery-3.6.0.js                         thebe-sage.js
jquery.js                               thebelab-helper.js
jupyter-sphinx.css                      thebelab.css
language_data.js                        underscore-1.13.1.js
logo_sagemath_black.svg                 underscore.js

Note that all mathjax files are in the subdirectory mathjax.

I think this is a regression.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change does not break the documentation, but has a problem which is the reason of the prefix mathjax/...

Note that all mathjax files are mixed with other static files while the original (without this PR) _static directory looks like this:

...

Note that all mathjax files are in the subdirectory mathjax.

I think this is a regression.

It's messy, but does it hurt anything?

That directory contains all of sphinx's build files and it's sphinx that decides what to put in there. We also see jquery.js and underscore.js (two independent projects) in that directory. If it were up to me the images, css, and javascript would all be in a separate directory, but I don't think moving just one javascript file into a subfolder makes things any cleaner or more consistent.

In any case, I changed it because the system mathjax may not live in a directory called "mathjax". Do you see an easy way around that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, _static directory is already a mess, but now mathjax becomes a mess too. mathjax comprises of many javascript files and directories containing data files. Mixing all of them with other files in _static directory makes the situation worse. We do (I did) look into _static directory from time to time to fix things. It's better to keep it tidy as much as we can.

@@ -374,7 +374,7 @@ def set_intersphinx_mappings(app, config):
if os.environ.get('SAGE_USE_CDNS', 'no') == 'yes':
mathjax_path = "https://cdn.jsdelivr.net/npm/mathjax@3/es5/tex-chtml.js"
else:
Copy link
Collaborator

@kwankyu kwankyu Aug 19, 2023

Choose a reason for hiding this comment

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

An "easy" fix would be to cut out the last path component of MATHJAX_DIR and prepend it to tex-chtml.js to make up mathjax_path.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean the lines L377,378.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No. This would not work...

AS_IF([test x$sage_spkg_install_mathjax = xyes], [
# Our spkg-src script adds an extra "mathjax"
SAGE_MATHJAX_DIR='${prefix}'/share/mathjax/mathjax
])
Copy link
Collaborator

Choose a reason for hiding this comment

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

There seems no easy fix. A trick is to add ".." to '${prefix}'/share/mathjax/mathjax here so that we use ".." to indicate that this is an spkg mathjax, and then use the indicator to differentiate how to tell sphinx where to find mathjax in conf.py. Yes, this is ugly...

Previously we copied mathjax to sphinx's _static output directory and
then referenced it with a path relative to _static. This commit
references it by absolute path on the filesystem, solving two
problems:

  1. It doesn't waste space; there are multiple _static directories,
     and mathjax was copied to all of them
  2. It avoids polluting _static with mathjax files

Closes: sagemath#30296
@orlitzky
Copy link
Contributor Author

I don't see any reason to copy mathjax into _static at all. I've replaced that last commit with one that uses the absolute path to mathjax. Not only should that avoid adding junk to _static, it saves a lot of space, because there are several _static directories.

@github-actions
Copy link

Documentation preview for this PR (built with commit ab1649c; changes) is ready! 🎉

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 21, 2023

I like that idea.

One concern is that the sage documentation becomes dependent on the mathjax external to it. This may break, for example, @haraldschilly's script for publishing the documenation to https://doc.sagemath.org, though perhaps this could be fixed on his side.

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 21, 2023

Or for the better, @haraldschilly can set SAGE_USE_CDNS=yes environment variable before he builds the documentation so that the built documentation does not depend on an external mathjax.

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 21, 2023

Could conda be supported? See #30296 (comment)

@orlitzky
Copy link
Contributor Author

Could conda be supported? See #30296 (comment)

I'm happy to support every other distro, I only need to know where mathjax is installed. What is $script_dir in that conda recipe?

@jhpalmieri
Copy link
Member

jhpalmieri commented Aug 21, 2023

I don't see any reason to copy mathjax into _static at all. I've replaced that last commit with one that uses the absolute path to mathjax. Not only should that avoid adding junk to _static, it saves a lot of space, because there are several _static directories.

See also #25111 (comment) for a related issue.

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 22, 2023

... What is $script_dir in that conda recipe?

Perhaps it is$(conda info --root)/conda-bld @mkoeppe ?

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 23, 2023

I will set this positive review tomorrow if there's no more input for mathjax installation paths, as it already improves the situation.

@orlitzky
Copy link
Contributor Author

Thank you!

@vbraun vbraun merged commit a3e6bb2 into sagemath:develop Aug 27, 2023
12 of 13 checks passed
@mkoeppe mkoeppe added this to the sage-10.2 milestone Aug 27, 2023
@kwankyu
Copy link
Collaborator

kwankyu commented Aug 28, 2023

This already broke the documentation previews for PRs.

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 28, 2023

An easy fix on the side of documentation previews is to use SAGE_USE_CDNS=yes as said in #36098 (comment)

When we do it, I suggest to use "SAGE_LIVE_DOC=yes" too (needs review #36144.)

@orlitzky
Copy link
Contributor Author

The docbuild is zipping up a subtree:

cp -r -L /sage/local/share/doc/sage/html/en/* ./docs
 # Zip everything for increased performance
zip -r docs.zip docs

and then publishing that somewhere. Now that tex-chtml.js isn't duplicated inside each _static folder, it's missing from the zip. We could probably make the deployment smart enough to fix the URL but for a Github preview I think SAGE_USE_CDNS=yes would be simpler.

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 28, 2023

Right. I think this

diff --git a/.github/workflows/doc-build.yml b/.github/workflows/doc-build.yml
index 3635980d15..eb4f81f303 100644
--- a/.github/workflows/doc-build.yml
+++ b/.github/workflows/doc-build.yml
@@ -82,6 +82,8 @@ jobs:
         # incremental docbuild may introduce broken links (inter-file references) though build succeeds
         run: |
           set -ex
+          export SAGE_USE_CDNS=yes
+          export SAGE_LIVE_DOC=yes
           mv /sage/local/share/doc/sage/html/en/.git /sage/.git-doc
           make doc-clean doc-uninstall sagelib-clean && git clean -fx src/sage
           mkdir -p /sage/local/share/doc/sage/html/en/ && mv /sage/.git-doc /sage/local/share/doc/sage/html/en/.git

would suffice (also for future live doc :-) @mkoeppe ?

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 30, 2023

I created an issue #36159 to track this.

@orlitzky orlitzky deleted the system-mathjax branch September 22, 2023 00:18
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.

System package information and spkg-configure for mathjax
5 participants