-
Notifications
You must be signed in to change notification settings - Fork 564
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
refactor main to for ease of integration #1948
Conversation
[wip] since there are a few references to BinExport2 that are in progress elsewhre. Next commit will remove them.
great initiative!! |
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.
looks great so far!!
Co-authored-by: Moritz <[email protected]>
this could go into v7, though with beta1 already out, i'm not sure if its appropriate. i think these changes are solid, but if they don't pass code review easily, then we can push them off to v8. |
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.
changes look good, it's quite a big change, but I'd prefer to get this into v7 now vs. doing a breaking v8 release next week
need to figure out what's going wrong with the |
Tests passing again (locally, at least). I don't have any other pending changes. Merging this before v7 works for me. |
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.
LGTM, THANK YOU!
let's merge this?! |
closes #1813
closes #1946
closes #1821
closes #1945
This PR reorganizes the logic found in
main()
because it was getting very large and hairy. The key change is to pull out chunks of code into their own functions, typically labelled*_from_cli()
, likeget_backend_from_cli()
. This localizes logic, which makes it easier for humans to read and make changes. It also means thatmain()
becomes much more reasonably sized (less than 100 lines, about one screenful).Typically we prefer to keep all input/output within main, calling library routines as necessary. This worked for a while, but
main
just became too large. The new "main routines" are expected to be used only withinmain
functions, either capa main or related scripts. They reach directly into the argparse arguments, write to stdout/stderr, and can instruct the program to exit prematurely. These functions should not be invoked from library code, and this is documented.Beyond copying code around, this PR refines our handling of the input file/format/backend. The logic for picking the format and backend is more consistent. Also, I've been careful to document that the input file is not necessarily the sample itself - cape/freeze/etc. inputs are not actually the sample.
While I was moving stuff around, I also addressed #1821, which moves a lot of potentially common routines from
capa.main
into the new namespacecapa.loader
, such asis_supported_os
,get_workspace
,get_extractor
, etc. These routines don't depend directly on our CLI argument handling and can reasonably be used by other library code.Here's an example of the changes needed to the freeze utility (they're basically 1 to 1):
But here's a more compelling example. The script needed to load rules and the input file, so we can replace a lot of the tedious exception handling with a consistent user interface (exceptions, status code, etc.):
TODO:
*_from_args
to*_from_cli
Checklist