Add pythemis_uninstall target to Makefile. Fixes #948

[sashimanu-san]

Added a pythemis_uninstall target to Makefile.

Checklist

• The coding guidelines are followed

[vixentael]

does it work? :D

[sashimanu-san]

does it work? :D

it does indeed!

[Lagovas]

I think will be safer to uninstall it with pip3 uninstall pythemis
Plus, we should test that it works. Can you add uninstall steps for our tests in github actions - https://github.com/cossacklabs/themis/blob/master/.github/workflows/test-python.yaml#L47? After finishing tests we can call pythemis_uninstall to be sure that this command works

[Lagovas]

additionally, will be great to delete files3.txt too. Due to this file generated by our Makefile, we should clean it too.

[vixentael]

but we install it manually here cd src/wrappers/themis/python/ && python3 setup.py install --record files3.txt, should we uninstall via pip3?

https://github.com/cossacklabs/themis/pull/949/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52R605

[Lagovas]

Yes, I revised one more time and your are right. System may haven't pip but be able to install packets with only python binary. So yes, probably we should delete with rm. But also we should delete files3.txt too. To return system to same state as it was before installation.

[shadinua]

Despite a rather common approach: to record created / to remove created, there are couple of thoughts:

• do we actually need -r flag for rm? as far as I can see there only files are expected, correct me please if I miss something
• the clause xargs rm -rf < files3.txt looks a bit dangerous for me; due to a mistake or even execution/write fault, we might get shorter path in the files3.txt than expected, which may lead to rm -rf / instead of rm -rf /usr/local/...; if we'd remove -r flag, as suggested above, I think it would be safely enough; if we cannot do that and expect deep directory structures, I'd add filter to prevent removing unexpected paths (for example, we may expect, that prefix would be always /usr/local/lib/python)

[ilammy]

Given that setup.py does not really have an uninstaller other that removing files recorded with --record, I think this will be an acceptable improvement. Better than having to remove files manually.

Normally, this installs an egg – just one file with an archive. However, I agree with @shadinua's concerns about removing stuff enumerated in some file. I'd suggest to improve the call by doing xargs -n 1 unlink. This should be portable between various UNIX-like systems and it shorter than, say, checking with shell that we remove files.

Another way would be to throw in --preserve-root, but that's a GNU-specific option. Since we expect only files to be removed, unlink fits the job well (removes one file, cannot remove directories). Still, xargs does not work for paths with spaces and newlines. But who uses those, right?

Ignore me. Simply not setting -r should be enough to remove only files.

---

I tend to think that python_uninstall should not remove files3.txt since that's just a part of the build, which should probably be removed by some clean instead. However, I wouldn't mind python_uninstall cleaning up the directory. Keep in mind though that python_install writes a bunch of other things as well besides files3.txt.

[vixentael]

unrelated to the PR

look, @ilammy oh no, rust has updated? or clippy? rust tests are failing 😱

error: unnecessary parentheses around match arm expression
   --> src/wrappers/themis/rust/src/error.rs:252:73

[ilammy]

As a long-term solution, we should think about migrating from setup.py completely to... well... whatever Python community considers the "modern way" this year. The one we use has been deprecated for about 10 years. With Guido stepping down, I wouldn't be surprised if they rip it out completely one day.

For python_install and python_uninstall, I think it would be acceptable to add a requirement to have pip installed locally, then do what RbThemis does in its targets: build a real package like you'd install from the repos and install that one locally. Not in this PR, a separate one would be nice. Gotta weight compatibility concerns separately.

[vixentael]

@sashimanu-san could you please take a look at the discussion?