Skip to content

Add pythemis_uninstall target to Makefile. Fixes #948 #949

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add pythemis_uninstall target to Makefile. Fixes #948 #949

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -611,6 +611,13 @@ endif
@echo -n "pythemis install "
@$(BUILD_CMD_)

pythemis_uninstall: CMD = cd src/wrappers/themis/python/ && xargs rm -rf < files3.txt
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@shadinua shadinua Sep 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator

@ilammy ilammy Sep 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

pythemis_uninstall:
@echo -n "pythemis uninstall "
@$(BUILD_CMD_)

uninstall: pythemis_uninstall

########################################################################
#
# Packaging Themis Core: Linux distributions
Expand Down