Closed mkoeppe closed 2 years ago
Commit: 18b4055
Description changed:
---
+++
@@ -1,4 +1,7 @@
This switch will disable the docbuild and installation of its many dependencies (sphinx...)
+
+In contrast to other packages, `ppl`'s docbuild is enabled unless explicitly disabled by setting `SAGE_SPKG_INSTALL_DOCS=no`.
+We conditionalize the dependency of `ppl` on this and also on `sphinx` being in the list of installed packages.
Split out from #31396.
Branch pushed to git repo; I updated commit sha1. New commits:
dfd23fb | build/pkgs/pplpy/dependencies: Fix up SAGE_SPKG_INSTALL_DOCS logic |
Author: Matthias Koeppe
Branch pushed to git repo; I updated commit sha1. New commits:
7307c75 | configure.ac (--disabled-notebook): Also disable nbclient |
Description changed:
---
+++
@@ -5,3 +5,5 @@
Split out from #31396.
+We also add another package to those that are disabled upon `--disable-notebook`.
+
If --disable-doc
is given, then should doc
be removed from the all-start
target, so that make
doesn't try to build the docs?
Also, after I used ./configure --disable-doc
, packages like alabaster
and sphinxcontrib-...
were built anyway.
Replying to @jhpalmieri:
If
--disable-doc
is given, then shoulddoc
be removed from theall-start
target, so thatmake
doesn't try to build the docs?
Yes, that's a good idea.
config.log
says (for example)
configure:45112: result: sphinx-4.2.0: standard, but disabled using configure option
but maybe the problem is that I ran make
instead of make build
?
Yes, probably the all-start
target pulled in the doc dependencies.
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
d13b8ae | configure.ac (--disable-doc): Disable also 'requests' and its dependencies, and direct sphinx dependencies |
4121cb0 | build/pkgs/pplpy/dependencies: Depend on sphinx only if SAGE_SPKG_INSTALL_DOCS!=no |
8ca1189 | build/pkgs/pplpy/dependencies: Fix up SAGE_SPKG_INSTALL_DOCS logic |
ccf6fb5 | configure.ac (--disabled-notebook): Also disable nbclient |
8e26780 | build/pkgs/sagemath_doc_{html,pdf}: New; delegate to them from targets doc-html etc. |
eeb8777 | m4/sage_spkg_collect.m4: Determine install tree for python3 via new trees.txt mechanism |
c8e19a0 | fixup |
5a4424b | build/make/Makefile.in (all-sage): Do not include packages installed in SAGE_DOCS |
67714b8 | Merge #31356 |
128de23 | configure.ac (--disable-doc): Disable the actual docbuild on 'make' |
Dependencies: #31356
Replying to @mkoeppe:
Replying to @jhpalmieri:
If
--disable-doc
is given, then shoulddoc
be removed from theall-start
target, so thatmake
doesn't try to build the docs?Yes, that's a good idea.
This is implemented now, via #31356
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
eb65e03 | build/pkgs/sagemath_doc_*/dependencies: Add sage_docbuild |
7989d87 | src/doc/Makefile: Handle errors from './sage --docbuild --all-documents' |
d64fb76 | build/make/Makefile.in: Undo reintroduction of SAGE_SKIP_PLOT_DIRECTIVE, SAGE_DOC_MATHJAX settings |
6afc0d5 | build/pkgs/sagemath_doc_html/spkg-install: Move handling of SAGE_DOC_JSMATH environment variable here from src/bin/sage-env |
b2cf40c | build/make/Makefile.in: Undo reintroduction of SAGE_SKIP_PLOT_DIRECTIVE, SAGE_DOC_MATHJAX settings (fixup) |
37a03bd | src/doc/en/developer/packaging.rst: Fix up section label |
fc89a66 | build/pkgs/sagemath_doc_pdf/spkg-install: Set SAGE_DOC_MATHJAX as a workaround |
fd298c4 | build/pkgs/sagemath_doc_pdf/dependencies: Depend on sagemath_doc_html |
b0593ea | Merge #31356 |
8a7027a | configure.ac (--disable-doc): Disable the actual docbuild on 'make' |
Rebased on top of updated #31356
Should we expect doctests to pass with ./configure --disable-doc
? I get
src/sage/misc/sagedoc.py # 8 doctests failed
src/sage_docbuild/utils.py # 5 doctests failed
src/sage_docbuild/__init__.py # 38 doctests failed
src/sage_docbuild/sphinxbuild.py # 13 doctests failed
src/sage/docs/conf.py # 1 doctest failed
which is not surprising, but should they be tagged so that they are only tested when sagemath_doc_html
is built?
git grep 'dochtml'
shows that we already seem to have an optional tag for this kind of thing.
We should probably replace it by # optional - sagemath_doc_html
and add the missing ones for the failing doctests.
Right, and then in the top-level Makefile
, this line needs to be changed:
TESTALL_FLAGS = --optional=sage,dochtml,optional,external,build
and maybe the last line in this block from src/sage/doctest/control.py
:
if have_git:
self.files.append(opj(SAGE_SRC, 'sage_setup'))
self.files.append(opj(SAGE_SRC, 'sage_docbuild'))
Perhaps define have_docs
appropriately and have a separate if have_docs
block. Either that or tag everything in sage_docbuild
.
Something like this?
diff --git a/src/sage/doctest/control.py b/src/sage/doctest/control.py
index 858d8cb567..8dbb693e22 100644
--- a/src/sage/doctest/control.py
+++ b/src/sage/doctest/control.py
@@ -727,13 +727,14 @@ class DocTestController(SageObject):
Doctesting ...
"""
opj = os.path.join
- from sage.env import SAGE_SRC, SAGE_DOC_SRC, SAGE_ROOT, SAGE_ROOT_GIT
+ from sage.env import SAGE_SRC, SAGE_DOC_SRC, SAGE_DOC, SAGE_ROOT, SAGE_ROOT_GIT
# SAGE_ROOT_GIT can be None on distributions which typically
# only have the SAGE_LOCAL install tree but not SAGE_ROOT
if SAGE_ROOT_GIT is not None:
have_git = os.path.isdir(SAGE_ROOT_GIT)
else:
have_git = False
+ have_docs = os.path.isdir(opj(SAGE_DOC, 'html', 'en', 'tutorial'))
def all_files():
self.files.append(opj(SAGE_SRC, 'sage'))
@@ -742,7 +743,8 @@ class DocTestController(SageObject):
# don't make sense to run outside a build environment
if have_git:
self.files.append(opj(SAGE_SRC, 'sage_setup'))
- self.files.append(opj(SAGE_SRC, 'sage_docbuild'))
+ if have_docs:
+ self.files.append(opj(SAGE_SRC, 'sage_docbuild'))
self.files.append(SAGE_DOC_SRC)
if self.options.all or (self.options.new and not have_git):
I think it would make more sense to just test whether opj(SAGE_SRC, 'sage_docbuild')
exists.
Same for sage_setup
.
I think we can get rid of the have_git
logic
By the way, with make distclean && ./configure --disable-doc && make build
, the various sphinx packages are still being built.
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
5d962fa | configure.ac (--disable-notebook): Also disable argon2_cffi |
da2f1cc | configure.ac (--disable-doc): New |
4f0fee0 | configure.ac (--disable-doc): Disable also 'requests' and its dependencies, and direct sphinx dependencies |
92d06dc | build/pkgs/pplpy/dependencies: Depend on sphinx only if SAGE_SPKG_INSTALL_DOCS!=no |
3b1f105 | build/pkgs/pplpy/dependencies: Fix up SAGE_SPKG_INSTALL_DOCS logic |
64273d1 | configure.ac (--disabled-notebook): Also disable nbclient |
b6167ca | configure.ac (--disable-doc): Disable the actual docbuild on 'make' |
Replying to @mkoeppe:
Rebased on top of updated #31356
... I had lost most of the commits in this rebase.
Fixed.
Replying to @mkoeppe:
I think it would make more sense to just test whether
opj(SAGE_SRC, 'sage_docbuild')
exists.Same for
sage_setup
.I think we can get rid of the
have_git
logic
Either that or test whether we can import sage_docbuild
. I think the directory will exist (in a source distribution) regardless, but we don't want to test if we haven't at least built sage_docbuild
. That seems cleaner than tagging everything in those files optional - sage_docbuild
.
I think the doctesting logic is that the source tree (in SAGE_ROOT/src) is preferred over installed sources, which are only used for falling back (for example for use in downstream packaging). But I agree, when falling back to installed sources, then importing sage_docbuild
and using its __path__
is the right solution
My point is that the typical doctest in sage_docbuild/*
starts with from sage_docbuild import ...
, so we shouldn't bother to test (as part of make testall
etc.) if we can't import sage_docbuild
. So I suggest something like:
diff --git a/src/sage/doctest/control.py b/src/sage/doctest/control.py
index 858d8cb567..2bf0a4beb4 100644
--- a/src/sage/doctest/control.py
+++ b/src/sage/doctest/control.py
@@ -740,9 +740,13 @@ class DocTestController(SageObject):
# Don't run these tests when not in the git repository; they are
# of interest for building sage, but not for runtime behavior and
# don't make sense to run outside a build environment
- if have_git:
+ if os.path.isdir(opj(SAGE_SRC, 'sage_setup')):
self.files.append(opj(SAGE_SRC, 'sage_setup'))
+ try:
+ import sage_docbuild
self.files.append(opj(SAGE_SRC, 'sage_docbuild'))
+ except ImportError:
+ pass
self.files.append(SAGE_DOC_SRC)
if self.options.all or (self.options.new and not have_git):
Ah ok, I agree. Same for sage_setup
then, I think
Branch pushed to git repo; I updated commit sha1. New commits:
3556181 | git grep -l '#.*optional.*dochtml' | xargs sed -i.bak 's/# optional - dochtml/# optional - sagemath_doc_html/' |
Branch pushed to git repo; I updated commit sha1. New commits:
4c11aab | sed -i.bak 's/dochtml/sagemath_doc_html/' Makefile src/bin/sage-runtests src/doc/en/developer/doctesting.rst |
Is it possible that SAGE_DOC_SRC
might be missing? We could do this:
diff --git a/src/sage/doctest/control.py b/src/sage/doctest/control.py
index 858d8cb567..86dbd24d1a 100644
--- a/src/sage/doctest/control.py
+++ b/src/sage/doctest/control.py
@@ -737,13 +737,22 @@ class DocTestController(SageObject):
def all_files():
self.files.append(opj(SAGE_SRC, 'sage'))
- # Don't run these tests when not in the git repository; they are
- # of interest for building sage, but not for runtime behavior and
- # don't make sense to run outside a build environment
- if have_git:
+ # Only test sage_setup and sage_docbuild if the relevant
+ # imports work. They may not work if not in a build
+ # environment or if the documentation build has been
+ # disabled.
+ try:
+ import sage_setup
self.files.append(opj(SAGE_SRC, 'sage_setup'))
+ except ImportError:
+ pass
+ try:
+ import sage_docbuild
self.files.append(opj(SAGE_SRC, 'sage_docbuild'))
- self.files.append(SAGE_DOC_SRC)
+ except ImportError:
+ pass
+ if os.path.isdir(SAGE_DOC_SRC):
+ self.files.append(SAGE_DOC_SRC)
if self.options.all or (self.options.new and not have_git):
self.log("Doctesting entire Sage library.")
New commits:
4c11aab | sed -i.bak 's/dochtml/sagemath_doc_html/' Makefile src/bin/sage-runtests src/doc/en/developer/doctesting.rst |
Can we autodetect sagemath_doc_html
as an optional tag to add for testing?
Yes, I think it should be autodetected already because sagemath_doc_html
is just an installed package
Replying to @jhpalmieri:
Is it possible that
SAGE_DOC_SRC
might be missing?
Yes, I think so, for example in downstream packaging of sage
Replying to @mkoeppe:
Yes, I think it should be autodetected already because
sagemath_doc_html
is just an installed package
Does that mean we shouldn't manually add it to TESTALL_FLAGS
and let it happen automatically?
Yes, that's a good point
Not sure how to implement TESTALL_NODOC_FLAGS
then
TESTALL_FLAGS = --optional=sage,sagemath_doc_html,optional,external,build
TESTALL_NODOC_FLAGS = --optional=sage,optional,external,build
(introduced in #31084)
Deprecate the nodoc
targets and tell people to use ./configure --disable-doc
instead?
Or leave it with the two FLAGS
being identical? Running make ptest-nodoc
runs make build
and so shouldn't build sagemath_doc_html
, so it shouldn't run tests with that flag, while make ptest
runs make
and so will build sagemath_doc_html
.
Thanks for the analysis, I like the 2nd option
Branch pushed to git repo; I updated commit sha1. New commits:
afa2f80 | Makefile: Get rid of TESTALL_NODOC_FLAGS |
This switch will disable the docbuild and avoid installing its many dependencies (sphinx...).
In contrast to other packages,
pplpy
's docbuild is enabled unless explicitly disabled by settingSAGE_SPKG_INSTALL_DOCS=no
. We move it to a separate packagepplpy_doc
and make it a dependency ofsagemath_doc_html
.Split out from #31396.
We also add
argon2_cffi
andnbclient
to the packages disabled upon--disable-notebook
.Depends on #29039
CC: @tobiasdiez @dimpase @jhpalmieri @sagetrac-tmonteil
Component: build: configure
Author: Matthias Koeppe, John Palmieri
Branch/Commit:
43221d0
Reviewer: John Palmieri, Matthias Koeppe
Issue created by migration from https://trac.sagemath.org/ticket/32759