pmret / papermario

Decompilation of Paper Mario (2000)
https://papermar.io
1.35k stars 128 forks source link

Update install_deps.sh #1187

Closed matejsmycka closed 6 months ago

matejsmycka commented 6 months ago
ethteck commented 6 months ago

I appreciate the intent behind this pr, but I don't think we should be automatically running pip install with the break system packages flag.

We have an open issue to rewrite this into a document that can be followed and doesn't have people mindlessly running sudo some script.sh. I think it'd be great to turn this into that document so we can explain things like why you might need to run pip install with this flag and what that means.

matejsmycka commented 6 months ago

I understand. While this initiative is great, users should be able to follow the guide or run a random script; taking responsibility for the script, the user should decide which approach he prefers.

About pip.

You are inherently breaking system dependencies using pip. Either you are using a proper virtual environment or want it to "just work". The current approach is halfway. My approach is to "just work". Maybe a better way would be to warn the user if command x doesn't work, use --break-dependencies. Or add poetry or pyenv.... to the install script.

Darxoon commented 6 months ago

If we are forcing everyone who wants to use the decomp to run a random shell script then we should really make sure it doesn't break people's systems. There's a reason break-system-packages is called the way it is

matejsmycka commented 6 months ago

Yes, but the current state still breaks them.

Darxoon commented 6 months ago

No? You will need to setup a venv before running install_deps.sh but it won't be able to harm the stability of your linux installation in general like break-system-packages has the potential to.

Remember that install_deps.sh is ran by a lot of people locally on their machines and using this flag carelessly like in your PR will inevitably make someone have to reinstall their OS because this flag has the potential to brick your entire linux installation if you're unfortunate.

matejsmycka commented 6 months ago

This is implicit info not mentioned anywhere afaik, but my PR is not a good way, i will close it

bates64 commented 6 months ago

See #1059