fix: Make sure to delete local temporary clones

[pawamoy]

Closes #709

I implemented that on GitPod.io but have a lot of test failures due to pexpect timeouts, and pkg_resources module not being found (it seems the env on gitpod is kinda broken). Opening to see how CI behaves.

[codecov-commenter]

Codecov Report

Merging #802 (629a93d) into master (bd766ba) will decrease coverage by 0.20%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #802      +/-   ##
==========================================
- Coverage   96.68%   96.48%   -0.21%     
==========================================
  Files          41       41              
  Lines        2959     2986      +27     
==========================================
+ Hits         2861     2881      +20     
- Misses         98      105       +7     

Flag	Coverage Δ
unittests	96.48% <100.00%> (-0.21%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
copier/main.py	94.54% <100.00%> (+0.09%)	⬆️
copier/template.py	98.14% <100.00%> (+0.08%)	⬆️
tests/test_vcs.py	100.00% <100.00%> (ø)
tests/test_tools.py	68.00% <0.00%> (-24.00%)	⬇️
copier/cli.py	96.47% <0.00%> (-1.18%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[pawamoy]

OK, I was able to fix the env on gitpod by upgrading pip and setuptools in both the pyenv version and the poetry venv.

[pawamoy]

Network failure in job ubuntu 3.8, re-running it should fix it.
I'll add a test as soon as possible and then mark the PR as ready.

[yajo]

Don't you think we need to fix this too?

copier/copier/vcs.py

Line 133 in 890ac86

location = tempfile.mkdtemp(prefix=f"{__name__}.clone.")

[pawamoy]

Oh, that's exactly what gets deleted by the added code. Sorry for not making it clear. I'll update the PR description accordingly.

[yajo]

Maybe a cleaner solution would be to add a destination argument to the clone() function. When calling it, we already provisioned the temporary directory. This way, the ownership of that variable belongs to the worker, and it would clean it up automatically, as I feel the cleanup should be in the temporary directory class better than in the worker.

[pawamoy]

I'm not sure to follow.

When calling it, we already provisioned the temporary directory.

Where would it be? I can't seem to find an attribute or property pointing to a temporary directory in the Template class.
If you meant to create the temporary directory above clone, in Template.local_abspath, then the temporary directory is still bound to the template and not the worker. If you meant to do this higher up in the chain again, for example in a method of the worker, then we should also refactor local_abspath as a regular method to be able to pass a destination argument. I'm OK to do this but am not sure of how this would impact the rest of the code flow.

the cleanup should be in the temporary directory class

That would be best indeed, but I don't see how we could use it in methods/properties as it's a context manager, and the temporary clone needs to exist for a longer time than the scope of the Template class' methods/properties.

[yajo]

I've reviewed this deeper and now I understand better what you said and why this is a good design. ❤️

I think it's good for a class to provision the paths it needs and clean them up, so let's move that logic to Template itself. It should be:

1. Define Template._cleanup().
2. Call it in Worker.__exit__().
3. Define Template._temp_clone as explained below.

This way, the template creates the paths and provides the way to remove it.

Adding context manager support to Worker also seems to be the way to go. TBH I was thinking that implementing __del__() would be better, but I've now learned that can lead to other problems.

Please don't forget to add a couple of tests:

1. The temporary clone is removed.
2. Copying from a local repo is not removed.

Also the API docs to use Worker as a context manager.

Thanks!