make clean destroys data with BUILDDIR = .

[georgefx]

make clean wipes the entire documentation if BUILDDIR = ..

The Makefile rule for clean is

.PHONY: clean
clean:
    rm -rf $(BUILDDIR)/*

Thus, it expands to rm -rf ./* and deletes everything in the current directory. Even worse, if BUILDDIR =.

Expected Behaviour
The spinx-doc makefile should only delete files and directories generated by sphinx-doc.

Proposed Fix
Generate a list of output files and directories in sphinx-build and delete them individually.

[lehmannro]

I think that's a classic case of Garbage In Garbage Out: We put everything inside of $BUILDDIR specifically so that we can just throw away the whole directory afterwards.

In order for your proposed fix to work, we'd need to record all files written during build. That'd require introducing a new file access layer for builders (they currently use raw open) and changing all of them. There are a number of third-party builders which we cannot influence.

I'd be willing to accept a sanity check for the Makefile which makes sure that BUILDDIR is well-formed.

[tk0miya]

I think your case is unusual for me. so, personally, recordings and sanity checks are no needed.

[lehmannro]

Well, we don't want to be the next bumblebee, so I think sanity checks wouldn't hurt. :-)

[tk0miya]

Ah, I understand that. The sanity check will guard users from suicide.
I will accept the changes if any pull request has come.

[georgefx]

I hacked together a sanity check which checks if BUILDDIR is a subdirectory of the Makefile directory (CURDIR), or more precisely, it checks if BUILDDIR is below CURDIR in the filesystem hierarchy and that BUILDIR and CURDIR are not identical. Here is the draft:

ABSBUILDDIR := $(abspath $(BUILDDIR))
SUBSTBUILDDIR := $(subst $(CURDIR),,$(ABSBUILDDIR))

BADBUILDDIR_ERRORMSG := BUILDDIR must be a directory below the root of the Sphinx documentation directory

INFO := $(info ABSBUILDDIR=$(ABSBUILDDIR))
INFO := $(info CURDIR=$(CURDIR))
INFO := $(info SUBSTBUILDDIR=$(SUBSTBUILDDIR))
ifeq ($(SUBSTBUILDDIR),$(ABSBUILDDIR)) 
    # CURDIR was not substituted in ABSBUILDDIR, thus ABSBUILDDIR is not below
    # CURDIR in the filesystem hierarchy.
    ERR := $(error FIRST $(BADBUILDDIR_ERRORMSG))
else ifeq ($(ABSBUILDDIR),$(CURDIR))
    # CURDIR and ABSBUILDDIR contain the same path.
    ERR := $(error SECOND $(BADBUILDDIR_ERRORMSG))
endif

I you know a cleaner way to do this in a Makefile, let me know. Otherwise, I would recommend to make the clean rule a call to the sphinx-build program. I.e.

.PHONY: clean
clean:
    $(SPHINXBUILD)  $(ALLSPHINXOPTS) --clean $(BUILDDIR)

This way, the sanity check on BUILDDIR can be implemented in python.
The sanity check should be done regardless of the --clean flag, so that one is protected of messing up the sphinx document root (or any other part of the fileystem) without a command to clean it later.

[lehmannro]

Hm, I think "must be a directory below the the root of Sphinx" is an unnecessarily strict requirement — writing to /tmp/... is perfectly fine. (FWIW, the easier phrasing of your proposal is "does ABSBUILDDIR start with CURDIR," which can be implemented using $(findstringfind,in).)

I see two options:

• Only do a very basic sanity check, e.g. ., .. and <empty string> (no path expansion, no nothing.) Everything else is shooting yourself into your foot voluntarily.
• Prohibit any parent path to the Sphinx root (ie. any prefix of it.) This nicely covers /, . as well.

[georgefx]

I agree on "BUILDDIR must be a directory below the the root of Sphinx" is an unnecessarily strict requirement.
However, BUILDDIR should not be allowed to be a parent of CURDIR. Tihnk about BUILDDIR := /home/georg. It is easy enough to do this by accident, i.e. a failed path completion, and the result of make clean is disastrous.
The first option (basic sanity check) is not good enough IMHO.

Regarding the second option, $(findstring find, in) is a better solution than $(subst ...) and ifeq. However, to clarify, both approaches do not implement "does ABSBUILDDIR start with CURDIR". Both just look for any occurence. This does not guarantee that ABSSBUILDIR is below CURDIR.
In summary, I do not see how "Prohibit any parent path to the Sphinx root (ie. any prefix of it.)" can be correctly implemented using make on-board tools. However, I think the test based on $(findstring...) comes close enough.
I will modify the Makefile accordingly and send a pull request on consent to this proposal.

[georgefx]

I implemented "Prohibit any parent path to the Sphinx root (ie. any prefix of it.)" using $(patsubst ...). patsubst patterns are actually anchored to the beginning of the tested string:

$(patsubst quaz,,/foo/quaz) -> /foo/quaz
$(patsubst %quaz,,/foo/quaz) ->
$(patsubst quaz%,,/foo/quaz) -> /foo/quaz
$(patsubst q%z,,/foo/quaz) -> /foo/quaz 
 $(patsubst /foo/q%z,,/foo/quaz) ->

Draft for testing if CURDIR is below BUILDIR and if CURDIR is equal to BUILDDIR:

ABSBUILDDIR := $(abspath $(BUILDDIR))
INFO := $(info BUILDDIR=$(BUILDDIR))
INFO := $(info ABSBUILDDIR=$(ABSBUILDDIR))
INFO := $(info CURDIR=$(CURDIR))
ifeq ($(patsubst $(ABSBUILDDIR)/%,,$(CURDIR)),)
    INFO := $(info ERROR CURDIR is below BUILDDIR)
else ifeq ($(ABSBUILDDIR),$(CURDIR))
    INFO := $(info ERROR CURDIR==BUILDDIR)
# special case BUILDDIR -> / does not match in in patsubst (pattern = //%), 
# handled here
else ifeq (/,$(ABSBUILDDIR))
    INFO := $(info ERROR BUILDDIR=/)
else
    INFO := $(info OK)
endif

Test:

BUILDDIR=build
ABSBUILDDIR=/home/georg/dev/make/build
CURDIR=/home/georg/dev/make
OK
- - - - - - -
BUILDDIR=.
ABSBUILDDIR=/home/georg/dev/make
CURDIR=/home/georg/dev/make
ERROR CURDIR==BUILDDIR
- - - - - - -
BUILDDIR=..
ABSBUILDDIR=/home/georg/dev
CURDIR=/home/georg/dev/make
ERROR CURDIR is below BUILDDIR
- - - - - - -
BUILDDIR=build/..
ABSBUILDDIR=/home/georg/dev/make
CURDIR=/home/georg/dev/make
ERROR CURDIR==BUILDDIR
- - - - - - -
BUILDDIR=build/../.
ABSBUILDDIR=/home/georg/dev/make
CURDIR=/home/georg/dev/make
ERROR CURDIR==BUILDDIR
- - - - - - -
BUILDDIR=../make/
ABSBUILDDIR=/home/georg/dev/make
CURDIR=/home/georg/dev/make
ERROR CURDIR==BUILDDIR
- - - - - - -
BUILDDIR=../make/.
ABSBUILDDIR=/home/georg/dev/make
CURDIR=/home/georg/dev/make
ERROR CURDIR==BUILDDIR
- - - - - - -
BUILDDIR=../make
ABSBUILDDIR=/home/georg/dev/make
CURDIR=/home/georg/dev/make
ERROR CURDIR==BUILDDIR
- - - - - - -
BUILDDIR=/
ABSBUILDDIR=/
CURDIR=/home/georg/dev/make
ERROR BUILDDIR=/
- - - - - - -
BUILDDIR=/tmp
ABSBUILDDIR=/tmp
CURDIR=/home/georg/dev/make
OK
- - - - - - -
BUILDDIR=/tmp/bar
ABSBUILDDIR=/tmp/bar
CURDIR=/home/georg/dev/make
OK
- - - - - - -
BUILDDIR=/home/quaz/bar
ABSBUILDDIR=/home/quaz/bar
CURDIR=/home/georg/dev/make
OK
- - - - - - -
BUILDDIR=/home/quaz/bar
ABSBUILDDIR=/home/quaz/bar
CURDIR=/home/georg/dev/make
OK
- - - - - - -
BUILDDIR=/home
ABSBUILDDIR=/home
CURDIR=/home/georg/dev/make
ERROR CURDIR is below BUILDDIR
- - - - - - -

Test Makefile (beware make metaprogramming):

BUILD_DIRS := build .  ..  build/.. build/../. ../make/ ../make/. ../make / /tmp /tmp/bar /home/quaz/bar /home/quaz/bar /home

define TEST_DIR_TEMPLATE  =
    ABSBUILDDIR := $$(abspath $$(BUILDDIR))
    INFO := $$(info BUILDDIR=$$(BUILDDIR))
    INFO := $$(info ABSBUILDDIR=$$(ABSBUILDDIR))
    INFO := $$(info CURDIR=$$(CURDIR))
    #INFO := $$(info test expands to ifeq ($$$$(patsubst $$(ABSBUILDDIR)/%,,$$(CURDIR)),))
    ifeq ($$(patsubst $$(ABSBUILDDIR)/%,,$$(CURDIR)),)
    INFO := $$(info ERROR CURDIR is below BUILDDIR)
    else ifeq ($$(ABSBUILDDIR),$$(CURDIR))
    INFO := $$(info ERROR CURDIR==BUILDDIR)

    # special case BUILDDIR -> / does not match in in patsubst (pattern = //%), 
    # handled here
    else ifeq (/,$$(ABSBUILDDIR))
    INFO := $$(info ERROR BUILDDIR=/)
    else
    INFO := $$(info OK)
    endif
    INFO := $$(info - - - - - - -)"
endef

$(foreach BUILDDIR, $(BUILD_DIRS), $(eval $(TEST_DIR_TEMPLATE))) 

[neuschaefer]

Arguably the simplest fix for this would be to remove the /*. rm -rf is already recursive.
(See also coreboot/coreboot@870f69e)

[tk0miya]

Fixed by #8450 instead. Closing.