Opened 3 years ago

Closed 2 weeks ago

#29097 closed enhancement (fixed)

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

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-9.7
Component: build Keywords:
Cc: dimpase, embray, jhpalmieri, slabbe, mjo, schilly Merged in:
Authors: John Palmieri, Matthias Koeppe Reviewers: Matthias Koeppe, John Palmieri
Report Upstream: N/A Work issues:
Branch: 72695aa (Commits, GitHub, GitLab) Commit: 72695aa2ee6c9c87e427933f11326d55f5ecc2c6
Dependencies: Stopgaps:

Status badges

Description (last modified by 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:

  • #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

Change History (46)

comment:1 Changed 2 years ago by mkoeppe

  • Description modified (diff)

comment:2 Changed 2 years ago by mkoeppe

  • Milestone changed from sage-9.1 to sage-9.2

comment:3 Changed 2 years ago by mkoeppe

  • Cc jhpalmieri slabbe added
  • Description modified (diff)

comment:4 Changed 2 years ago by mkoeppe

  • Cc mjo added

comment:5 Changed 2 years ago by mkoeppe

  • Dependencies set to #29793

comment:6 Changed 2 years ago by jhpalmieri

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.

comment:7 Changed 2 years ago by jhpalmieri

  • Branch set to u/jhpalmieri/clean2uninstall

comment:8 Changed 2 years ago by jhpalmieri

  • Commit set to 5203e5043ab29eb9fcf88256e8164ebe1d7bf0f3

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:

5203e50trac 29097: change "make SPKG-clean" to "make SPKG-uninstall".

comment:9 follow-up: Changed 2 years ago by mjo

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,

  1. Rename doc-output-clean to doc-uninstall (already done).
  2. Drop doc-clean
  3. Rename doc-src-clean to doc-clean
  4. 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)

comment:10 in reply to: ↑ 9 Changed 2 years ago by jhpalmieri

Replying to mjo:

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,

  1. Rename doc-output-clean to doc-uninstall (already done).
  2. Drop doc-clean
  3. Rename doc-src-clean to doc-clean
  4. 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.

comment:11 Changed 2 years ago by mkoeppe

  • Cc schilly added

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?

comment:12 Changed 2 years ago by mkoeppe

  • Description modified (diff)

comment:13 Changed 2 years ago by jhpalmieri

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.

comment:14 Changed 2 years ago by mkoeppe

  • Dependencies changed from #29793 to #29793, #29868

comment:15 Changed 2 years ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:16 Changed 23 months ago by mkoeppe

  • Dependencies changed from #29793, #29868 to #29793, #29868, #29386
  • Description modified (diff)

comment:17 Changed 17 months ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-9.4

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.

comment:18 Changed 14 months ago by mkoeppe

  • Description modified (diff)

comment:19 Changed 13 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5

comment:20 Changed 8 months ago by mkoeppe

  • Milestone changed from sage-9.5 to sage-9.6

comment:21 Changed 4 months ago by mkoeppe

  • Milestone changed from sage-9.6 to sage-9.7

comment:22 Changed 4 weeks ago by mkoeppe

  • Description modified (diff)

comment:23 Changed 4 weeks ago by mkoeppe

  • Authors set to John Palmieri, Matthias Koeppe
  • Dependencies #29793, #29868, #29386 deleted
  • Description modified (diff)

comment:24 Changed 4 weeks ago by mkoeppe

  • Branch changed from u/jhpalmieri/clean2uninstall to u/mkoeppe/clean2uninstall

comment:25 Changed 4 weeks ago by mkoeppe

  • Commit changed from 5203e5043ab29eb9fcf88256e8164ebe1d7bf0f3 to 65a56047d5933990cc3a65ef437b530a08aa3f7b
  • Status changed from new to needs_review

New commits:

2633cc9build/sage_bootstrap/uninstall.py: Do not refuse to uninstall packages that do not exist in the source tree
dbc61b3build/sage_bootstrap/uninstall.py: Do not override the stamp file location from environment variable
3a5ea93build/make/Makefile.in: New implicit targets %-SAGE_LOCAL-uninstall, %-SAGE_VENV-uninstall
65a5604trac 29097: change "make SPKG-clean" to "make SPKG-uninstall".

comment:26 Changed 4 weeks ago by mkoeppe

  • Description modified (diff)

comment:27 Changed 3 weeks ago by mkoeppe

  • Description modified (diff)

comment:28 Changed 3 weeks ago by mkoeppe

  • Cc nbruin added
  • Description modified (diff)

comment:29 Changed 3 weeks ago by jhpalmieri

  • Cc nbruin removed
  • Description modified (diff)

What is the purpose of the %-SAGE_LOCAL-uninstall and %-SAGE_VENV-uninstall targets? Where are they used?

comment:30 Changed 3 weeks ago by jhpalmieri

  • Description modified (diff)

comment:31 Changed 3 weeks ago by mkoeppe

  • Description modified (diff)

comment:32 Changed 3 weeks ago by mkoeppe

Fixed the description to explain it

comment:33 Changed 3 weeks ago by mkoeppe

Ah, but I can probably do better

comment:34 Changed 3 weeks ago by git

  • Commit changed from 65a56047d5933990cc3a65ef437b530a08aa3f7b to 46096069a2b56db02e600ccb542e3823d2d7c322

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

4609606build/make/Makefile.in: Also add implicit rule %-uninstall

comment:35 Changed 3 weeks ago by mkoeppe

  • Description modified (diff)

comment:36 follow-up: Changed 3 weeks ago by jhpalmieri

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.

comment:37 follow-up: Changed 3 weeks ago by 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?

comment:38 in reply to: ↑ 36 Changed 3 weeks ago by mkoeppe

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

comment:39 Changed 3 weeks ago by git

  • Commit changed from 46096069a2b56db02e600ccb542e3823d2d7c322 to e9b30bb31428885a6bebbf6a56f7491431046554

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

e9b30bbbuild/sage_bootstrap/uninstall.py: Update comment

comment:40 Changed 3 weeks ago by git

  • Commit changed from e9b30bb31428885a6bebbf6a56f7491431046554 to fef04d0cf798c43d33f40a2f4aa5ad04395ed6e9

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

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

comment:41 in reply to: ↑ 37 Changed 3 weeks ago by mkoeppe

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

comment:42 Changed 3 weeks ago by git

  • Commit changed from fef04d0cf798c43d33f40a2f4aa5ad04395ed6e9 to 9693865b1eabba0fd125679c1d25e13365d9211c

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

9693865build/make/Makefile.in: Clarify SAGE_I_TARGETS

comment:43 Changed 3 weeks ago by jhpalmieri

  • Reviewers set to John Palmieri
  • Status changed from needs_review to positive_review

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:

  • build/sage_bootstrap/uninstall.py

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

comment:44 Changed 3 weeks ago by git

  • Commit changed from 9693865b1eabba0fd125679c1d25e13365d9211c to 72695aa2ee6c9c87e427933f11326d55f5ecc2c6
  • Status changed from positive_review to needs_review

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

72695aabuild/sage_bootstrap/uninstall.py: Fix typo in docstring

comment:45 Changed 3 weeks ago by mkoeppe

  • Reviewers changed from John Palmieri to Matthias Koeppe, John Palmieri
  • Status changed from needs_review to positive_review

Thank you!

comment:46 Changed 2 weeks ago by vbraun

  • Branch changed from u/mkoeppe/clean2uninstall to 72695aa2ee6c9c87e427933f11326d55f5ecc2c6
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.