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

deps: make socks and serial optional #2245

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from

Conversation

Arusekk
Copy link
Member

@Arusekk Arusekk commented Jul 29, 2023

No description provided.

@peace-maker
Copy link
Member

Do you want to remove them from the required packages in the pyproject.yml too? Maybe move them to a "all" group or similar?
https://setuptools.pypa.io/en/latest/userguide/dependency_management.html#optional-dependencies

@Arusekk
Copy link
Member Author

Arusekk commented Jul 31, 2023 via email

pass
except IOError as e:
if not isinstance(e, socket.error):
raise
Copy link
Member

Choose a reason for hiding this comment

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

This appears to render the following code useless, since a ProxyError is not a socket.error?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a bit tricky, so I am not sure I got it right:

  • We want to reraise all socks.ProxyErrors. We also want to silence all socket.errors (not sure why, but w/e, this is what the code has been doing so far).
  • On py2 socket.error is a child class of IOError, which is also the base class for socks.ProxyError. Therefore the following code is useless indeed.
  • On py3 socket.error is a deprecated alias for OSError, which is also aliased as IOError, and is therefore the base class for socks.ProxyError. The first check will always fail (so not sure whether we want to keep it), so the second check for socks.ProxyError is still needed, although I think we could also get away without importing socks, but through inspecting the exception's __mro__ for classes matching c.__module__ == "socks" and c.__name__ == "ProxyError". This is not an expensive import however, and an error path anyway, so I think we can afford some extra time spent during actually importing the module.

Another idea I have is to cache the last exception anyway, and raise it just as socket.create_connection does. And I think this will be the best way in the end.

@Arusekk Arusekk force-pushed the optional-deps branch 3 times, most recently from 389e0b9 to 3c1b453 Compare August 20, 2023 10:58
@peace-maker
Copy link
Member

Hm, we should log the pip install pwntools[full] command and the exact missing library in the error message when users try to use such a feature.

To avoid confusion with the change of pip install pwntools not installing everything.

@Arusekk
Copy link
Member Author

Arusekk commented Aug 21, 2023 via email

@peace-maker
Copy link
Member

The typer package now provides a typer-slim package which doesn't contain heavy dependencies.
https://github.com/tiangolo/typer?tab=readme-ov-file#typer-slim

Maybe that is an option.

@Arusekk
Copy link
Member Author

Arusekk commented Aug 7, 2024

Okay, maybe it would be doable, but not in this release. I would be happy to already have the deps de facto optional (i.e. pwntools not breaking if something from this list is missing) though.

@Arusekk Arusekk added this to the 5.0.0 milestone Dec 11, 2024
@peace-maker peace-maker added the dependencies Pull requests that update a dependency file label Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants