Skip to content
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.

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

Fix: various errors during installation #28

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

Conversation

digglife
Copy link

@digglife digglife commented May 13, 2023

  1. Add Missing space before the git url. This problem causes git clone failure, so the whole share object building process doesn't work.

  2. Remove sudo statements. If the package path is not writable to current user, then it should not be. We should let user to decide whether to use sudo instead of prompt password in the middle of installing (Actually install to system site-packages with admin privilege is a bad practice). Also, if user is using virtualenv or install with --user, it's perfectly fine without sudo.

  3. Avoid echoing the output because system output is unpredictable. It could be multiple lines, so os.system("echo ", output.decode("utf-8")) could end up executing the following shell command

    echo the first line
    the second line
    the third line

    the second line and the third line will be executed as normal shell command.

    We don't need to use echo to print out the result.

  4. os.exit(1) should be sys.exit(1)

@digglife
Copy link
Author

digglife commented Jun 4, 2023

Hi @kmozurkewich @ayush-srivastava-03

Could you please review this fix? If you are no longer maintaining this project, do you know who should we contact for issue report or PR? Thanks!

@kmozurkewich
Copy link
Member

Hello!

Thank you for the PR. Will get it in the queue for the end of next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants