build/make/Makefile.in: Rename make targets SPKG-clean to SPKG-uninstall
mkoeppe opened this issue · 53 comments
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:
- #29310 - introduced
make doc-uninstall - #33705
make doc-cleanshould remove inventory, doctrees - #29386 Install script packages via
sage-spkg - #29626 ./configure --enable-SPKG does not work for optional pip packages
- #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
Description changed:
---
+++
@@ -2,3 +2,4 @@
But these targets *uninstall* packages from `@prefix@=$SAGE_LOCAL`.
+Likewise "doc-output-clean" should be renamed to "doc-uninstall".Description changed:
---
+++
@@ -3,3 +3,8 @@
Likewise "doc-output-clean" should be renamed to "doc-uninstall".
+
+
+Also useful would be phony targets of the form `SPKG-deps` that would install all prerequisite targets of `SPKG`, but not `SPKG` itself.
+
+Also related: #29626 ./configure --enable-SPKG does not work for optional pip packagesThere 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.
Branch: u/jhpalmieri/clean2uninstall
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". |
Commit: 5203e50
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,
- Rename
doc-output-cleantodoc-uninstall(already done). - Drop
doc-clean - Rename
doc-src-cleantodoc-clean - Change the suggestion to run both
doc-cleananddoc-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)
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-cleandoes bothdoc-src-clean doc-output-clean. I suggest,
- Rename
doc-output-cleantodoc-uninstall(already done).- Drop
doc-clean- Rename
doc-src-cleantodoc-clean- Change the suggestion to run both
doc-cleananddoc-uninstallThat 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.
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?
Description changed:
---
+++
@@ -8,3 +8,5 @@
Also useful would be phony targets of the form `SPKG-deps` that would install all prerequisite targets of `SPKG`, but not `SPKG` itself.
Also related: #29626 ./configure --enable-SPKG does not work for optional pip packages
+
+See also: #29868 pip-installable packages `sage-doc-html`, `sage-doc-pdf`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.
Description changed:
---
+++
@@ -1,5 +1,7 @@
Cleaning means to remove build artifacts from the directory `@builddir@` (= SAGE_ROOT).
But these targets *uninstall* packages from `@prefix@=$SAGE_LOCAL`.
+
+(Part of this has been done in #29386.)
Likewise "doc-output-clean" should be renamed to "doc-uninstall".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.
Description changed:
---
+++
@@ -6,6 +6,8 @@
Likewise "doc-output-clean" should be renamed to "doc-uninstall".
+Also `make docbuild-clean` does not do anything at all.
+
Also useful would be phony targets of the form `SPKG-deps` that would install all prerequisite targets of `SPKG`, but not `SPKG` itself.
Description changed:
---
+++
@@ -1,16 +1,9 @@
Cleaning means to remove build artifacts from the directory `@builddir@` (= SAGE_ROOT).
But these targets *uninstall* packages from `@prefix@=$SAGE_LOCAL`.
-(Part of this has been done in #29386.)
+Related:
+- #29310 - introduced `make doc-uninstall`
+- #29386 Install script packages via `sage-spkg`
+- #29626 ./configure --enable-SPKG does not work for optional pip packages
+- #29868 pip-installable packages `sage-doc-html`, `sage-doc-pdf`
-
-Likewise "doc-output-clean" should be renamed to "doc-uninstall".
-
-Also `make docbuild-clean` does not do anything at all.
-
-
-Also useful would be phony targets of the form `SPKG-deps` that would install all prerequisite targets of `SPKG`, but not `SPKG` itself.
-
-Also related: #29626 ./configure --enable-SPKG does not work for optional pip packages
-
-See also: #29868 pip-installable packages `sage-doc-html`, `sage-doc-pdf`Author: John Palmieri, Matthias Koeppe
Description changed:
---
+++
@@ -1,8 +1,13 @@
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.)
+
+We also add implicit rules that allow users to uninstall packages that no longer exist in the source tree. This is for #34203
+
Related:
- #29310 - introduced `make doc-uninstall`
+- #33705 `make doc-clean` should remove inventory, doctrees
- #29386 Install script packages via `sage-spkg`
- #29626 ./configure --enable-SPKG does not work for optional pip packages
- #29868 pip-installable packages `sage-doc-html`, `sage-doc-pdf`Changed branch from u/jhpalmieri/clean2uninstall to u/mkoeppe/clean2uninstall
New commits:
2633cc9 | build/sage_bootstrap/uninstall.py: Do not refuse to uninstall packages that do not exist in the source tree |
dbc61b3 | build/sage_bootstrap/uninstall.py: Do not override the stamp file location from environment variable |
3a5ea93 | build/make/Makefile.in: New implicit targets %-SAGE_LOCAL-uninstall, %-SAGE_VENV-uninstall |
65a5604 | trac 29097: change "make SPKG-clean" to "make SPKG-uninstall". |
Description changed:
---
+++
@@ -1,7 +1,8 @@
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.)
+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 for #34203
Description changed:
---
+++
@@ -4,7 +4,7 @@
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 for #34203
+We also add implicit rules that allow users to uninstall packages that no longer exist in the source tree. This is 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:
- #29310 - introduced `make doc-uninstall`Description changed:
---
+++
@@ -4,7 +4,7 @@
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 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).
+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:
- #29310 - introduced `make doc-uninstall`Description changed:
---
+++
@@ -4,7 +4,7 @@
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).
+We also add implicit rules that allow users to uninstall packages that no longer exist in the source tree. This is for #34203
Related:
- #29310 - introduced `make doc-uninstall`What is the purpose of the %-SAGE_LOCAL-uninstall and %-SAGE_VENV-uninstall targets? Where are they used?
Description changed:
---
+++
@@ -4,7 +4,7 @@
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 for #34203
+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:
- #29310 - introduced `make doc-uninstall`Description changed:
---
+++
@@ -4,7 +4,7 @@
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).
+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-SAGE_VENV-uninstall` (works).
Related:
- #29310 - introduced `make doc-uninstall`Fixed the description to explain it
Ah, but I can probably do better
Branch pushed to git repo; I updated commit sha1. New commits:
4609606 | build/make/Makefile.in: Also add implicit rule %-uninstall |
Description changed:
---
+++
@@ -4,7 +4,7 @@
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-SAGE_VENV-uninstall` (works).
+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:
- #29310 - introduced `make doc-uninstall`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.
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?
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
Branch pushed to git repo; I updated commit sha1. New commits:
e9b30bb | build/sage_bootstrap/uninstall.py: Update comment |
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 |
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_LOCALbut alsoSAGE_VENV? Or point out in the docstring that the variablesage_localshould be eitherSAGE_LOCALorSAGE_VENV?
Done, please take a look
Branch pushed to git repo; I updated commit sha1. New commits:
9693865 | build/make/Makefile.in: Clarify SAGE_I_TARGETS |
Reviewer: John Palmieri
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.
"""
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 |
Thank you!
Changed reviewer from John Palmieri to Matthias Koeppe, John Palmieri
Changed branch from u/mkoeppe/clean2uninstall to 72695aa