build/make/Makefile.in: Rename make targets SPKG-clean to SPKG-uninstall

[mkoeppe]

Cleaning means to remove build artifacts from the directory @builddir@ (= SAGE_ROOT).
But these targets uninstall packages from @prefix@=$SAGE_LOCAL.

Hence we rename SPKG-clean targets to SPKG-uninstall (but keep SPKG-clean as an alias.) This also unifies the targets between normal and script packages (the latter used -uninstall already).

We also add implicit rules that allow users to uninstall packages that no longer exist in the source tree. This is preparation for #34203. To test this, switch to the branch of #33530 and build one of the packages added, for example make fastjsonschema. Switch back to current develop and try to remove it (does not work). Switch to the branch here and run make fastjsonschema-uninstall (works).

Related:

• "make distclean" should not run "./configure" #29310 - introduced make doc-uninstall
• "make doc-clean" should remove inventory, doctrees #33705 make doc-clean should remove inventory, doctrees
• Install script and pip packages via sage-spkg #29386 Install script packages via sage-spkg
• Remove ./configure --enable-SPKG options for pip packages (for which this is not implemented) #29626 ./configure --enable-SPKG does not work for optional pip packages
• pip-installable packages sagemath-doc-src, sagemath-doc-inventory, sagemath-doc-html, sagemath-doc-pdf #29868 pip-installable packages sage-doc-html, sage-doc-pdf

CC: @dimpase @embray @jhpalmieri @seblabbe @orlitzky @haraldschilly

Component: build

Author: John Palmieri, Matthias Koeppe

Branch/Commit: 72695aa

Reviewer: Matthias Koeppe, John Palmieri

Issue created by migration from https://trac.sagemath.org/ticket/29097

[mkoeppe]

Dependencies: #29793

[jhpalmieri]

comment:6

There is no reason for any deprecation here, is there? I have a branch that I'm testing out now to do the change "clean -> uninstall", but not the new phony targets SPKG-deps.

[jhpalmieri]

Branch: u/jhpalmieri/clean2uninstall

[jhpalmieri]

comment:8

Here is a branch to test out. I wasn't sure about the instruction when docbuilding fails:

    Note: incremental documentation builds sometimes cause spurious
    error messages. To be certain that these are real errors, run
    "make doc-clean" first and try again.

There could be artifacts that make doc-uninstall won't clean up, so I think I want to keep this as make doc-clean. I don't know if it should also suggest running ./bootstrap, but that's a separate issue.

---

New commits:

5203e50

trac 29097: change "make SPKG-clean" to "make SPKG-uninstall".

[jhpalmieri]

Commit: 5203e50

[orlitzky]

comment:9

There could be artifacts that make doc-uninstall won't clean up, so I think I want to keep this as make doc-clean

Right now doc-clean does both doc-src-clean doc-output-clean. I suggest,

0. Rename doc-output-clean to doc-uninstall (already done).
1. Drop doc-clean
2. Rename doc-src-clean to doc-clean
3. Change the suggestion to run both doc-clean and doc-uninstall

That way the targets wind up with the right names, and the suggestion doesn't effectively change.

NB: in this hunk the doc-src-clean was dropped (I don't know which is correct):

-doc-html-no-plot: doc-clean $(DOC_DEPENDENCIES)
+doc-html-no-plot: doc-uninstall $(DOC_DEPENDENCIES)

[jhpalmieri]

comment:10

Replying to @orlitzky:

There could be artifacts that make doc-uninstall won't clean up, so I think I want to keep this as make doc-clean

Right now doc-clean does both doc-src-clean doc-output-clean. I suggest,

0. Rename doc-output-clean to doc-uninstall (already done).
1. Drop doc-clean
2. Rename doc-src-clean to doc-clean
3. Change the suggestion to run both doc-clean and doc-uninstall

That way the targets wind up with the right names, and the suggestion doesn't effectively change.

My only reluctance is that I am very used to typing make doc-clean when working with tickets involving docbuilding. I don't know how many others are in the same habit, but we would all have to change that habit.

NB: in this hunk the doc-src-clean was dropped (I don't know which is correct):

-doc-html-no-plot: doc-clean $(DOC_DEPENDENCIES)
+doc-html-no-plot: doc-uninstall $(DOC_DEPENDENCIES)

This should be okay: the reason to clean the docs when toggling doc-html-no-plot is because inclusion (or not) of plots is cached in the doc output (in local/share/doc/sage/inventory or maybe .../doctrees). So doc-uninstall should be good enough. It works for me, at least.

[mkoeppe]

comment:11

For the doc-... cleaning/uninstalling targets, I would like to understand first whether everything that we put in $SAGE_LOCAL/share/doc/sage/ should really be installed there.

Are doctrees and inventory needed in the installation at all? Do other Python packages install inventory files? Or is this just something that is reused and updated in an incremental docbuild?

[jhpalmieri]

comment:13

The inventory files are intended to allow for cross-references among different pieces of the documentation: see https://www.sphinx-doc.org/en/master/usage/extensions/intersphinx.html and set_intersphinx_mapping in src/sage/docs/conf.py. It allows the tutorial (for example) to refer to the reference manual. For example, the reference :ref:`sage.rings.padics.tutorial` in tour_numtheory.rst in the tutorial points to the reference manual, and I think this uses the inventory files to construct the link when building the documentation.

According to https://www.sphinx-doc.org/en/master/man/sphinx-build.html, the doctrees are used to cache the parsed source files before writing output. Once the output has been written, they aren't needed, so I guess they could be moved somewhere else, and I'm guessing the same for the inventory files, but I think they are all needed when building the docs.

[mkoeppe]

Changed dependencies from #29793 to #29793, #29868

[mkoeppe]

Changed dependencies from #29793, #29868 to #29793, #29868, #29386

[mkoeppe]

comment:17

Sage development has entered the release candidate phase for 9.3. Setting a new milestone for this ticket based on a cursory review of ticket status, priority, and last modification date.

[jhpalmieri]

comment:36

Thank you for the explanation and the new target; that now makes sense.

Not from this branch: I don't understand SAGE_I_TARGETS. List of targets that can be run using `sage -i` or `sage -f`. The list contains only sagelib and doc, so is the implication that one shouldn't use sage -i sagetex? As far as I can tell, SAGE_I_TARGETS is only used in the list target.

Regarding this change in sage_bootstrap/uninstall.py:

     # The default path to this directory; however its value should be read
     # from the environment if possible
     spkg_inst = pth.join(sage_local, 'var', 'lib', 'sage', 'installed')
-    spkg_inst = os.environ.get('SAGE_SPKG_INST', spkg_inst)

Is the point that we now have two possible locations for the installation files, either SAGE_LOCAL or SAGE_VENV, so we can't use the single variable SAGE_SPKG_INST? I think the comment about reading the value from the environment should probably be removed or changed.

Otherwise things look good.

[jhpalmieri]

comment:37

And if you're going to fix the comment about environment variables, maybe also fix the docstring for the function to not refer to just SAGE_LOCAL but also SAGE_VENV? Or point out in the docstring that the variable sage_local should be either SAGE_LOCAL or SAGE_VENV?

[mkoeppe]

comment:38

Replying to @jhpalmieri:

Regarding this change in sage_bootstrap/uninstall.py:

     # The default path to this directory; however its value should be read
     # from the environment if possible
     spkg_inst = pth.join(sage_local, 'var', 'lib', 'sage', 'installed')
-    spkg_inst = os.environ.get('SAGE_SPKG_INST', spkg_inst)

Is the point that we now have two possible locations for the installation files, either SAGE_LOCAL or SAGE_VENV, so we can't use the single variable SAGE_SPKG_INST?

Yes.

I think the comment about reading the value from the environment should probably be removed or changed.

Thanks, I'll fix that

[sagetrac-git]

Changed commit from 4609606 to e9b30bb

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

e9b30bb

build/sage_bootstrap/uninstall.py: Update comment

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

fef04d0

build/sage_bootstrap/uninstall.py: Clarify that the sage_local argument is SAGE_LOCAL or SAGE_VENV

[sagetrac-git]

Changed commit from e9b30bb to fef04d0

[mkoeppe]

comment:41

Replying to @jhpalmieri:

And if you're going to fix the comment about environment variables, maybe also fix the docstring for the function to not refer to just SAGE_LOCAL but also SAGE_VENV? Or point out in the docstring that the variable sage_local should be either SAGE_LOCAL or SAGE_VENV?

Done, please take a look

[sagetrac-git]

Changed commit from fef04d0 to 9693865

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

9693865

build/make/Makefile.in: Clarify SAGE_I_TARGETS

[jhpalmieri]

Reviewer: John Palmieri

[jhpalmieri]

comment:43

Looks good to me. I don't have access to my usual computer so I can't push to trac right now. If you feel like fixing it, there is a typo:

diff --git a/build/sage_bootstrap/uninstall.py b/build/sage_bootstrap/uninstall.py
index feb91ead158..1ce039921fc 100644
--- a/build/sage_bootstrap/uninstall.py
+++ b/build/sage_bootstrap/uninstall.py
@@ -51,7 +51,7 @@ PKGS = pth.join(SAGE_ROOT, 'build', 'pkgs')
 
 def uninstall(spkg_name, sage_local, keep_files=False, verbose=False):
     """
-    Given a package name and path to an installation tree (SAGE_LOCAL or SAGE_VENV,
+    Given a package name and path to an installation tree (SAGE_LOCAL or SAGE_VENV),
     uninstall that package from that tree if it is currently installed.
     """
 

[sagetrac-git]

Changed commit from 9693865 to 72695aa

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

72695aa

build/sage_bootstrap/uninstall.py: Fix typo in docstring

[mkoeppe]

comment:45

Thank you!

[mkoeppe]

Changed reviewer from John Palmieri to Matthias Koeppe, John Palmieri

[vbraun]

Changed branch from u/mkoeppe/clean2uninstall to 72695aa