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

Add support for virtualenv #1119

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Add support for virtualenv #1119

wants to merge 4 commits into from

Conversation

ValekoZ
Copy link
Collaborator

@ValekoZ ValekoZ commented Jun 5, 2024

Add a gef.virtualenv_path config var, and activate the provided virtualenv at startup.

This could be used by gef-extras to install python dependencies cleanly.

Add a `gef.virtualenv_path` config var, and activate it at startup.
Copy link

github-actions bot commented Jun 5, 2024

🤖 Coverage update for 65330a7 🟢

Old New
Commit 1b6f46a 65330a7
Score 71.6907% 71.6907% (0)

Copy link

github-actions bot commented Jun 5, 2024

🤖 Coverage update for f5f1bc5 🟢

Old New
Commit 1b6f46a f5f1bc5
Score 71.6907% 71.697% (0.0063)

Copy link

github-actions bot commented Jun 5, 2024

🤖 Coverage update for f5f1bc5 🟢

Old New
Commit 1b6f46a f5f1bc5
Score 71.6907% 71.6907% (0)

Copy link

github-actions bot commented Jun 5, 2024

🤖 Coverage update for 7c32602 🟢

Old New
Commit 1b6f46a 7c32602
Score 71.6907% 71.7064% (0.0157)

Copy link

github-actions bot commented Jun 5, 2024

🤖 Coverage update for 7c32602 🟢

Old New
Commit 1b6f46a 7c32602
Score 71.6907% 71.7095% (0.0188)

Copy link

github-actions bot commented Jun 9, 2024

🤖 Coverage update for 7e97732 🟢

Old New
Commit 1b6f46a 7e97732
Score 71.6798% 71.6798% (0)

Copy link

github-actions bot commented Jun 9, 2024

🤖 Coverage update for 7e97732 🟢

Old New
Commit 1b6f46a 7e97732
Score 71.6798% 71.752% (0.0722)

Copy link
Owner

@hugsy hugsy left a comment

Choose a reason for hiding this comment

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

TL;DR: I think this needs to be re-worked at least by adding way more test cases and documentation.

As I mentioned, a lot of complications come when dealing with seperate environments, and so it should be tested way more, for instance:

  • we need to check the venv is running at least for the minimum Python version we supported
  • what happens when a user uses a python version different from that with gdb
  • what happens when a user switches between venv
  • etc.

In addition, this should be exhaustively documented - not just the gef config option, but how to use gef in a virtual env because it is 100% certain users will come with such questions (and/or bad issue reports about it)

The pyenv was done hastily which came to bite us down the line, let's not repeat that mistake

Comment on lines +9952 to +9954
if path:
activate_script_path = pathlib.Path(path)/"bin"/"activate_this.py"
exec(open(activate_script_path).read(), {'__file__': activate_script_path})
Copy link
Owner

Choose a reason for hiding this comment

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

Avoid cascading by returning first on error

Suggested change
if path:
activate_script_path = pathlib.Path(path)/"bin"/"activate_this.py"
exec(open(activate_script_path).read(), {'__file__': activate_script_path})
if not path:
return
activate_script_path = pathlib.Path(path)/"bin"/"activate_this.py"
exec(open(activate_script_path).read(), {'__file__': activate_script_path})

Comment on lines +9952 to +9954
if path:
activate_script_path = pathlib.Path(path)/"bin"/"activate_this.py"
exec(open(activate_script_path).read(), {'__file__': activate_script_path})
Copy link
Owner

Choose a reason for hiding this comment

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

Also simpler

        exec(activate_script_path.read_text(), {'__file__': activate_script_path})

def load_virtualenv(self, new_path: Optional[pathlib.Path] = None):
path = new_path or gef.config["gef.virtualenv_path"]
if path:
activate_script_path = pathlib.Path(path)/"bin"/"activate_this.py"
Copy link
Owner

Choose a reason for hiding this comment

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

Here path is already known to be of type pathlib.Path, no neeed for a 2nd allocation,

    activate_script_path = path / "bin/activate_this.py"
    assert activate_script_path.is_file()

@@ -9854,6 +9854,9 @@ def __init__(self) -> None:
plugins_dir = GefSetting("", str, "Autoload additional GEF commands from external directory", hooks={"on_write": [GefSetting.no_spaces, ]})
plugins_dir.add_hook("on_changed", [lambda _, new_val: GefSetting.must_exist(new_val), lambda _, new_val: self.load_extra_plugins(new_val), ])
gef.config["gef.extra_plugins_dir"] = plugins_dir
venv_path = GefSetting("", str, "Path to the virtualenv used by GEF", hooks={"on_write": [GefSetting.no_spaces, ]})
venv_path.add_hook("on_changed", [lambda _, new_val: GefSetting.must_exist(new_val), lambda _, new_val: self.load_virtualenv(new_val), ])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure why we do it above, but you shuld be able to just add on_changed as an extra key in the hooks arg for the constructor.

@@ -9943,6 +9946,13 @@ def load_plugins_from_directory(plugin_directory: pathlib.Path):
dbg(f"Loading extra plugins from directory={directory}")
return load_plugins_from_directory(directory)


def load_virtualenv(self, new_path: Optional[pathlib.Path] = None):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if this makes sense as a method here

gdb.execute(f"gef config gef.virtualenv_path {self.venv_path}")

res = gdb.execute("pi __import__('numpy').test()", to_string=True)
assert 'NumPy version' in res
Copy link
Collaborator

Choose a reason for hiding this comment

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

double quotes

Copy link

stale bot commented Aug 9, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. You can reopen it by adding a comment to this issue.

@stale stale bot added the stale label Aug 9, 2024
@hugsy hugsy removed the stale label Aug 9, 2024
@hugsy
Copy link
Owner

hugsy commented Aug 9, 2024

Assigning to shush state-bot

@hugsy
Copy link
Owner

hugsy commented Nov 3, 2024

@ValekoZ Update on this PR?

@ValekoZ
Copy link
Collaborator Author

ValekoZ commented Nov 10, 2024

I'll work on it later, I don't have much time these days (but it should get better in the coming weeks :D)

@ValekoZ ValekoZ marked this pull request as draft November 10, 2024 16:02
@hugsy hugsy added this to the next milestone Nov 10, 2024
@hugsy
Copy link
Owner

hugsy commented Nov 10, 2024

I'll work on it later, I don't have much time these days (but it should get better in the coming weeks :D)

No problem at all 😄

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.

3 participants