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

jhpalmieri · 2020-03-10T22:30:11Z

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.

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

CC: @jhpalmieri @mkoeppe @slel

Component: build

Keywords: quiet, speed

Author: John Palmieri

Branch: e28f093

Reviewer: Matthias Koeppe

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

The text was updated successfully, but these errors were encountered:

mkoeppe · 2020-03-29T02:19:42Z

comment:1

See also:

Merge build/make/deps into build/make/Makefile.in #29098 - Merge build/make/deps into build/make/Makefile.in
build/make/Makefile.in: Rename make targets SPKG-clean to SPKG-uninstall #29097 - build/make/Makefile.in: Rename make targets SPKG-clean to SPKG-uninstall
which could be done at the same time.

jhpalmieri · 2020-04-18T20:32:33Z

comment:3

This could presumably be fixed by #29316.

slel · 2020-09-07T20:39:13Z

comment:5

Replying to @jhpalmieri:

This could presumably be fixed by #29316.

Apparently not.

slel · 2020-09-07T20:45:12Z

Changed keywords from none to quiet, speed

mkoeppe · 2021-02-13T20:51:01Z

comment:7

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

mkoeppe · 2021-10-24T17:56:04Z

comment:9

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

jhpalmieri · 2021-10-27T20:27:34Z

comment:10

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.

jhpalmieri · 2021-10-27T22:00:24Z

comment:11

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"

mkoeppe · 2021-10-29T02:32:21Z

comment:12

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)

jhpalmieri · 2021-10-29T05:18:12Z

Branch: u/jhpalmieri/distclean

jhpalmieri · 2021-10-29T05:20:35Z

comment:14

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`

jhpalmieri · 2021-10-29T05:20:35Z

Commit: c7cfd31

jhpalmieri · 2021-10-29T05:26:45Z

comment:15

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.

mkoeppe · 2021-10-29T05:32:20Z

comment:16

Try $(CURDIR)

sagetrac-git · 2021-10-29T16:03:42Z

Changed commit from c7cfd31 to d3d1405

sagetrac-git · 2021-10-29T16:03:42Z

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

`d3d1405`	`trac 29310: use CURDIR`

jhpalmieri · 2021-10-29T16:04:54Z

comment:18

Replying to @mkoeppe:

Try $(CURDIR)

Thanks, done.

jhpalmieri · 2021-12-16T00:05:47Z

comment:20

I think this is ready for review.

jhpalmieri · 2021-12-16T00:06:01Z

Author: John Palmieri

mkoeppe · 2021-12-17T06:33:11Z

comment:22

+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

sagetrac-git · 2021-12-17T19:44:59Z

Changed commit from d3d1405 to 67d69da

sagetrac-git · 2021-12-17T19:44:59Z

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 "?="`

jhpalmieri · 2021-12-17T19:45:19Z

comment:24

Fixed, thanks.

mkoeppe · 2021-12-20T23:47:52Z

comment:25

+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...

sagetrac-git · 2021-12-21T03:22:01Z

Changed commit from 67d69da to e28f093

sagetrac-git · 2021-12-21T03:22:01Z

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

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

jhpalmieri · 2021-12-21T03:22:54Z

comment:27

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

mkoeppe · 2021-12-23T05:14:28Z

comment:28

LGTM and seems to work well.

mkoeppe · 2021-12-23T05:32:13Z

Reviewer: Matthias Koeppe

jhpalmieri · 2021-12-23T19:06:05Z

comment:30

Thank you!

vbraun · 2021-12-28T21:15:33Z

Changed branch from u/jhpalmieri/distclean to e28f093

mkoeppe · 2021-12-28T21:18:28Z

Changed commit from e28f093 to none

jhpalmieri added this to the sage-9.1 milestone Mar 10, 2020

jhpalmieri added c: build labels Mar 10, 2020

mkoeppe modified the milestones: sage-9.1, sage-9.2 Apr 15, 2020

mkoeppe modified the milestones: sage-9.2, sage-9.3 Aug 29, 2020

This comment has been minimized.

Sign in to view

mkoeppe modified the milestones: sage-9.3, sage-9.4 Feb 13, 2021

mkoeppe modified the milestones: sage-9.4, sage-9.5 Jul 19, 2021

mkoeppe modified the milestones: sage-9.5, sage-9.6 Dec 14, 2021

jhpalmieri added the s: needs review label Dec 16, 2021

mkoeppe added s: positive review and removed s: needs review labels Dec 23, 2021

vbraun removed the s: positive review label Dec 28, 2021

vbraun closed this as completed in da90b23 Dec 28, 2021

mkoeppe modified the milestones: sage-9.6, sage-9.5 Dec 28, 2021

jhpalmieri mentioned this issue Jan 9, 2022

Fix references to doc-clean #33104

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jhpalmieri commented Mar 10, 2020

mkoeppe commented Mar 29, 2020

jhpalmieri commented Apr 18, 2020

slel commented Sep 7, 2020

slel commented Sep 7, 2020

This comment has been minimized.

mkoeppe commented Feb 13, 2021

mkoeppe commented Oct 24, 2021

jhpalmieri commented Oct 27, 2021

jhpalmieri commented Oct 27, 2021

mkoeppe commented Oct 29, 2021

jhpalmieri commented Oct 29, 2021

jhpalmieri commented Oct 29, 2021

jhpalmieri commented Oct 29, 2021

jhpalmieri commented Oct 29, 2021

mkoeppe commented Oct 29, 2021

sagetrac-git mannequin commented Oct 29, 2021

sagetrac-git mannequin commented Oct 29, 2021

jhpalmieri commented Oct 29, 2021

jhpalmieri commented Dec 16, 2021

jhpalmieri commented Dec 16, 2021

mkoeppe commented Dec 17, 2021

sagetrac-git mannequin commented Dec 17, 2021

sagetrac-git mannequin commented Dec 17, 2021

jhpalmieri commented Dec 17, 2021

mkoeppe commented Dec 20, 2021

sagetrac-git mannequin commented Dec 21, 2021

sagetrac-git mannequin commented Dec 21, 2021

jhpalmieri commented Dec 21, 2021

mkoeppe commented Dec 23, 2021

mkoeppe commented Dec 23, 2021

jhpalmieri commented Dec 23, 2021

vbraun commented Dec 28, 2021

mkoeppe commented Dec 28, 2021

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

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

Comments

jhpalmieri commented Mar 10, 2020

mkoeppe commented Mar 29, 2020

jhpalmieri commented Apr 18, 2020

slel commented Sep 7, 2020

slel commented Sep 7, 2020

This comment has been minimized.

mkoeppe commented Feb 13, 2021

mkoeppe commented Oct 24, 2021

jhpalmieri commented Oct 27, 2021

jhpalmieri commented Oct 27, 2021

mkoeppe commented Oct 29, 2021

jhpalmieri commented Oct 29, 2021

jhpalmieri commented Oct 29, 2021

jhpalmieri commented Oct 29, 2021

jhpalmieri commented Oct 29, 2021

mkoeppe commented Oct 29, 2021

sagetrac-git mannequin commented Oct 29, 2021

sagetrac-git mannequin commented Oct 29, 2021

jhpalmieri commented Oct 29, 2021

jhpalmieri commented Dec 16, 2021

jhpalmieri commented Dec 16, 2021

mkoeppe commented Dec 17, 2021

sagetrac-git mannequin commented Dec 17, 2021

sagetrac-git mannequin commented Dec 17, 2021

jhpalmieri commented Dec 17, 2021

mkoeppe commented Dec 20, 2021

sagetrac-git mannequin commented Dec 21, 2021

sagetrac-git mannequin commented Dec 21, 2021

jhpalmieri commented Dec 21, 2021

mkoeppe commented Dec 23, 2021

mkoeppe commented Dec 23, 2021

jhpalmieri commented Dec 23, 2021

vbraun commented Dec 28, 2021

mkoeppe commented Dec 28, 2021