#29310 closed enhancement (fixed)
"make distclean" should not run "./configure"
Reported by:  John Palmieri  Owned by:  

Priority:  major  Milestone:  sage9.5 
Component:  build  Keywords:  quiet, speed 
Cc:  John Palmieri, Matthias Köppe, Samuel Lelièvre  Merged in:  
Authors:  John Palmieri  Reviewers:  Matthias Koeppe 
Report Upstream:  N/A  Work issues:  
Branch:  e28f093 (Commits, GitHub, GitLab)  Commit:  
Dependencies:  Stopgaps: 
Description (last modified by )
As the summary says. It may be good enough to move the targets clean
, sagelibclean
, buildclean
, docclean
, docsrcclean
, and docoutputclean
from build/make/deps
to the toplevel Makefile
.
This would make things faster and more quiet.
Related:
Change History (32)
comment:1 Changed 3 years ago by
comment:2 Changed 3 years ago by
Milestone:  sage9.1 → sage9.2 

comment:4 Changed 2 years ago by
Milestone:  sage9.2 → sage9.3 

comment:5 Changed 2 years ago by
comment:6 Changed 2 years ago by
Cc:  John Palmieri Matthias Köppe Samuel Lelièvre added 

Description:  modified (diff) 
Keywords:  quiet speed added 
comment:7 Changed 2 years ago by
Milestone:  sage9.3 → sage9.4 

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.
comment:8 Changed 19 months ago by
Milestone:  sage9.4 → sage9.5 

comment:9 Changed 15 months ago by
Also, in 9.4.beta5 I am getting with make distclean
:
/bin/sh: /Users/mkoeppe/s/sage/sagerebasing/worktreerebase/build/pkgs/sagelib/spkguninstall: No such file or directory
comment:10 Changed 15 months ago by
Can we naively define SAGE_ROOT
, SAGE_SHARE
, SAGE_LOCAL
, and SAGE_SRC
in the toplevel Makefile, and then move the various clean
targets to that file? I think that will solve the problem mentioned in this ticket, but I don't know how careful we need to be about setting those variables.
comment:11 Changed 15 months ago by
I'm confused by the logic here. I think that the issues arise from these lines in Makefile
:
# Defer unknown targets to build/make/Makefile %:: @if [ x relocateonce.py ]; then ./relocateonce.py; fi $(MAKE) build/make/Makefile stop +build/bin/sagelogger \ "cd build/make && ./install '$@'" logs/install.log
So that's why I want to move the cleaning targets to the toplevel Makefile
. SAGE_LOCAL should be set by the ./configure
script, depending on the prefix
argument (for example), but we want make distclean
to work regardless of whether ./configure
has been run. Part of make distclean
already does rm rf local
. We have the following uses of SAGE_
variables in the cleaning targets:
rm rf "$(SAGE_LOCAL)/var/tmp/sage/build"
cd "$(SAGE_SRC)" && (do some stuff)
cd "$(SAGE_ROOT)/build/pkgs/sagelib/src/" && rm rf build
(cd "$(SAGE_ROOT)/build/pkgs/sage_docbuild/src" && rm rf build)
(cd "$(SAGE_ROOT)/build/pkgs/sage_setup/src" && rm rf build)
cd "$(SAGE_SRC)/doc" && $(MAKE) clean
rm rf "$(SAGE_SHARE)/doc/sage"
comment:12 Changed 15 months ago by
Moving the cleaning targets to the toplevel Makefile could make sense.
Most of the above lines really don't need configured variables.
For cleaning within SAGE_LOCAL
(and SAGE_VENV
):
As you say, make distclean
unconditionally removes all of SAGE_ROOT/local
. But if prefix
has been used to set SAGE_LOCAL
to something else, that tree is not removed (and it should not; see discussion in #21775).
So it only matters what is to be done when SAGE_LOCAL
has been set to something else.
To run a line such as rm rf "$(SAGE_LOCAL)/var/tmp/sage/build"
, we could try to source SAGE_ROOT/src/bin/sagesrcenvconfig
, which defines the configured SAGE_LOCAL
and SAGE_VENV
. If that fails (because configure
has not run), do nothing.
I'm not sure about rm rf "$(SAGE_SHARE)/doc/sage"
. This not only removes build artifacts such as the inventory files, but it also uninstalls the built HTML documentation. I don't think this should be done by any target called ...clean
.
(see #29097  build/make/Makefile.in
: Rename make targets SPKGclean
to SPKGuninstall
)
comment:13 Changed 15 months ago by
Branch:  → u/jhpalmieri/distclean 

comment:14 Changed 15 months ago by
Commit:  → c7cfd313ace5d6ae7a847a8d6fb69d20951527d6 

Here is an attempt. A few comments:
 I added the line
if [ d "$(SAGE_SRC)" ]; then \
because I kept making mistakes which resulted inSAGE_SRC
not being set, and then the ensuing lines did things likecd $(SAGE_SRC) && rm rf build
which ended up being bad. So this is a safety net.  The new target
docuninstall
is not currently used anywhere. Maybe it should be.
New commits:
c7cfd31  trac 29310: move various ...clean targets to toplevel Makefile

comment:15 Changed 15 months ago by
Oh, and I used $(shell pwd)
when setting SAGE_ROOT
because (with what I think is a nonGNU make on OS X) it was the only way I found to set SAGE_ROOT
and have it expand immediately when being set. When I tried something like
SAGE_ROOT = $$(pwd) cleantest: echo $(SAGE_ROOT)
then make cleantest
would print
echo $(pwd) /Users/palmieri/....
With the current version, if I add the same target cleantest
, then make cleantest
prints
echo /Users/... /Users/...
It seems safest to have SAGE_ROOT
defined once, not defined in terms of pwd
which could change in the middle of a recipe.
comment:17 Changed 15 months ago by
Commit:  c7cfd313ace5d6ae7a847a8d6fb69d20951527d6 → d3d14055243d3ef2d20a061061be952aca82f34b 

Branch pushed to git repo; I updated commit sha1. New commits:
d3d1405  trac 29310: use CURDIR

comment:19 Changed 14 months ago by
Milestone:  sage9.5 → sage9.6 

comment:21 Changed 14 months ago by
Authors:  → John Palmieri 

comment:22 Changed 14 months ago by
+SAGE_ROOT ?= $(CURDIR) +SAGE_SRC ?= $(SAGE_ROOT)/src
I think it would be safer to just use =
instead of ?=
. We don't want environment variable settings to be used here
comment:23 Changed 14 months ago by
Commit:  d3d14055243d3ef2d20a061061be952aca82f34b → 67d69da24a88e4d357ce834cb7fd229b93bf770e 

comment:25 Changed 13 months ago by
+clean: + @echo "Deleting package build directories..." + if [ x "$(SAGE_SRC)"/bin/sageenvconfig ]; then \ + . "$(SAGE_SRC)"/bin/sageenvconfig; \ + rm rf "$(SAGE_LOCAL)/var/tmp/sage/build"; \ + fi
The reference to SAGE_LOCAL
does not work  the syntax $(...) is referring to a Make variable, but you want a shell variable. So $$...
should be used instead.
Also, for extra safety, best to test that the variable is nonempty before passing it to rm rf
...
comment:26 Changed 13 months ago by
Commit:  67d69da24a88e4d357ce834cb7fd229b93bf770e → e28f0934ce9619c4eb13b45b015165d8804829c2 

Branch pushed to git repo; I updated commit sha1. New commits:
e28f093  trac 29310: use $$SAGE_LOCAL instead of $(SAGE_LOCAL) 

comment:27 Changed 13 months ago by
It was okay because that branch wasn't being run anyway, since sageenvconfig
is not executable. Fixed now.
comment:28 Changed 13 months ago by
Status:  needs_review → positive_review 

LGTM and seems to work well.
comment:29 Changed 13 months ago by
Reviewers:  → Matthias Koeppe 

comment:31 Changed 13 months ago by
Branch:  u/jhpalmieri/distclean → e28f0934ce9619c4eb13b45b015165d8804829c2 

Resolution:  → fixed 
Status:  positive_review → closed 
comment:32 Changed 13 months ago by
Commit:  e28f0934ce9619c4eb13b45b015165d8804829c2 

Milestone:  sage9.6 → sage9.5 
See also:
which could be done at the same time.