-
-
Notifications
You must be signed in to change notification settings - Fork 8
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
Remove Nix #11
Remove Nix #11
Conversation
Feedback from Roger and others was to go "vanilla" python project. Therefore remove Nix, Poetry and go back to published versions of dependencies such as apsw and lief
* updated readme to reflect removal of Nix * added new API sql.py to execute sqlelf directly * added some new tests for the new API * reworked the cli.py to invoke the new API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please move the pip install -e .[dev]
part of the readme above the makefile stuff per the comment.
everything else is optional!
❯ python3 -m venv venv | ||
❯ source venv/bin/activate | ||
❯ pip install . | ||
❯ sqlelf /usr/bin/python3 -- \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: it'd be nice to change this to either /usr/bin/env python3
or $VIRTUAL_ENV/bin/python3
because user's python3
might be anywhere on the file system
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you talking about the sqlelf
invocation or the -m venv
one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, the sqlelf
invocation because the first command line argument is the absolute path of a python executable
|
||
# If none of the inputs are valid files, simply return | ||
if len(filenames) == 0: | ||
sys.exit("No valid ELF files were provided") | ||
|
||
binaries: list[Binary] = [Binary(filename) for filename in filenames] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this related to removing nix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I downgraded LIEF as part of the nix removal and it changed the API a bit.
for section in binary.sections: | ||
yield { | ||
"path": binary_path, | ||
"path": binary_name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: why binary_name
instead of path? is it just the base name, not a full path? that's odd given dictionary's key remains path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was called name on the object model even though its the path.
It's just an annoying thing about LIEF
Feedback from @rogerbinns and others was to go "vanilla" python project.
Therefore remove Nix, Poetry and go back to published versions of dependencies such as apsw and lief