"make distclean" should not run "./configure"

[John Palmieri]

As the summary says. It may be good enough to move the targets clean, sagelib-clean, build-clean, doc-clean, doc-src-clean, and doc-output-clean from build/make/deps to the top-level Makefile.

This would make things faster and more quiet.

Related:

• #30258: Have configure run quiet if started by make in silent mode
• #29097: Rename make targets SPKG-clean to SPKG-uninstall

[Matthias Köppe]

See also:

• #29098 - Merge build/make/deps into build/make/Makefile.in
• #29097 - build/make/Makefile.in: Rename make targets SPKG-clean to SPKG-uninstall

which could be done at the same time.

[John Palmieri]

This could presumably be fixed by #29316.

[Samuel Lelièvre]

Replying to jhpalmieri:

This could presumably be fixed by #29316.

Apparently not.

[Matthias Köppe]

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

[Matthias Köppe]

Also, in 9.4.beta5 I am getting with make distclean:

/bin/sh: /Users/mkoeppe/s/sage/sage-rebasing/worktree-rebase/build/pkgs/sagelib/spkg-uninstall: No such file or directory

[John Palmieri]

Can we naively define SAGE_ROOT, SAGE_SHARE, SAGE_LOCAL, and SAGE_SRC in the top-level 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.

[John Palmieri]

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 relocate-once.py ]; then ./relocate-once.py; fi
	$(MAKE) build/make/Makefile --stop
	+build/bin/sage-logger \
		"cd build/make && ./install '$@'" logs/install.log

So that's why I want to move the cleaning targets to the top-level 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"

[Matthias Köppe]

Moving the cleaning targets to the top-level 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/sage-src-env-config, 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 SPKG-clean to SPKG-uninstall)

[John Palmieri]

Here is an attempt. A few comments:

• I added the line if [ -d "$(SAGE_SRC)" ]; then \ because I kept making mistakes which resulted in SAGE_SRC not being set, and then the ensuing lines did things like cd $(SAGE_SRC) && rm -rf build which ended up being bad. So this is a safety net.
• The new target doc-uninstall is not currently used anywhere. Maybe it should be.

---

New commits:

​c7cfd31

trac 29310: move various ...-clean targets to top-level Makefile

[John Palmieri]

Oh, and I used $(shell pwd) when setting SAGE_ROOT because (with what I think is a non-GNU 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)

clean-test:
    echo $(SAGE_ROOT)

then make clean-test would print

echo $(pwd)
/Users/palmieri/....

With the current version, if I add the same target clean-test, then make clean-test 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.

[Matthias Köppe]

Try $(CURDIR)

[git]

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

​d3d1405

trac 29310: use CURDIR

[John Palmieri]

Replying to mkoeppe:

Try $(CURDIR)

Thanks, done.

[John Palmieri]

I think this is ready for review.

[Matthias Köppe]

+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

[git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​f53be61	trac 29310: move various ...-clean targets to top-level Makefile
​3a7712e	trac 29310: use CURDIR
​67d69da	trac 29310: use "=" instead of "?="

[John Palmieri]

Fixed, thanks.

[Matthias Köppe]

+clean:
+	@echo "Deleting package build directories..."
+	if [ -x "$(SAGE_SRC)"/bin/sage-env-config ]; then \
+	    . "$(SAGE_SRC)"/bin/sage-env-config; \
+            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...

[git]

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

​e28f093

trac 29310: use $$SAGE_LOCAL instead of $(SAGE_LOCAL) --

[John Palmieri]

It was okay because that branch wasn't being run anyway, since sage-env-config is not executable. Fixed now.

[Matthias Köppe]

LGTM and seems to work well.

[John Palmieri]

Thank you!