make clean removes venv directory in source path unconditionally

[stweil]

Ideally it would only remove files created by the build process.

If the user has created the venv directory manually and added own content, removing that directory might be bad.

Makefile could check whether that virtual environment was active outside of make and skip the removal then.

[kba]

Makefile could check whether that virtual environment was active outside of make and skip the removal then.

Sounds reasonable 👍 @bertsky any consequences we're not seeing?

[bertsky]

make clean removes venv directory in source path unconditionally
Ideally it would only remove files created by the build process.

If the user has created the venv directory manually and added own content, removing that directory might be bad.

In the current rule definition, clean only cleanes the path ocrd_all uses by default to create a new venv of its own. It does not use VIRTUAL_ENV from the environment (which might be user-defined).

If both coincide, then it is removed. So you want to back out of that decision?

Makefile could check whether that virtual environment was active outside of make and skip the removal then.

What do you propose? Replace the VIRTUAL_ENV ?= with some form of ifdef?

[stweil]

What do you propose?

--- a/Makefile
+++ b/Makefile
@@ -69,7 +69,7 @@ check:
 
 clean: # add more prerequisites for clean below
        $(RM) -r $(SUB_VENV)/*
-       $(RM) -r $(CURDIR)/venv # deliberately not using VIRTUAL_ENV here
+       if ! command -v activate; then $(RM) -r $(VIRTUAL_ENV; fi
        $(RM) -r $(HOME)/.parallel/semaphores/id-ocrd_all_git/
 
 define HELP

Maybe a comment can also be added: "Remove virtual environment only if it was not active outside of the make context.".

[kba]

•   if ! command -v activate; then $(RM) -r $(VIRTUAL_ENV; fi
  

I don't think activate will be detected as a command because while in $PATH if $VIRTUAL_ENV is activated it is not executable.

Wrong, it does work, should have checked.

[stweil]

Improved version:

diff --git a/Makefile b/Makefile
index d10e36a..cd7751c 100644
--- a/Makefile
+++ b/Makefile
@@ -69,7 +69,8 @@ check:
 
 clean: # add more prerequisites for clean below
        $(RM) -r $(SUB_VENV)/*
-       $(RM) -r $(CURDIR)/venv # deliberately not using VIRTUAL_ENV here
+       # Remove virtual environment only if it was not active outside of the make context.
+       command -v activate >/dev/null || $(RM) -r $(VIRTUAL_ENV)
        $(RM) -r $(HOME)/.parallel/semaphores/id-ocrd_all_git/
 
 define HELP

[stweil]

Wrong, it does work, should have checked.

I was also surprised about that.

[bertsky]

-       $(RM) -r $(CURDIR)/venv # deliberately not using VIRTUAL_ENV here
+       # Remove virtual environment only if it was not active outside of the make context.
+       command -v activate >/dev/null || $(RM) -r $(VIRTUAL_ENV)

That's not correct. This would even snatch away an externally controlled venv passed in via VIRTUAL_ENV (either from shell variable or by make argument or in local.mk). You should at least use $(CURDIR)/venv. But a better criterion than activate (which could be anything) is the ifdef approach suggested above.

[stweil]

Well, it does not snatch if the external VIRTUAL_ENV was active when make clean was called.

But I agree that using macros which come from outside with $(RM) -r can be dangerous, so feel free to use $(CURDIR)/venv to avoid trouble coming from someone running make clean VIRTUAL_ENV=/.