-
Notifications
You must be signed in to change notification settings - Fork 133
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
Fix circular dependencies by making ofrak_angr
and ofrak_capstone
optional for ofrak_core
tests
#420
Fix circular dependencies by making ofrak_angr
and ofrak_capstone
optional for ofrak_core
tests
#420
Conversation
requirements-test.txt
make develop
in core into two stages.
@whyitfor @rbs-jacob does this approach make sense? |
Specifically, this results in something like develop:
$(MAKE) -C ofrak_type STAGE=1 develop
$(MAKE) -C ofrak_io STAGE=1 develop
$(MAKE) -C ofrak_patch_maker STAGE=1 develop
$(MAKE) -C ofrak_core STAGE=1 develop
$(MAKE) -C ofrak_angr STAGE=1 develop
$(MAKE) -C ofrak_capstone STAGE=1 develop
$(MAKE) -C frontend STAGE=1 develop
$(MAKE) -C ofrak_core STAGE=2 develop where |
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.
This approach could work, but it introduces some complexity that I'm not sure is worth it.
Could the same end result be achieved by modifying the ofrak develop
target to ensure proper install order?
Something like:
develop:
$(PIP) install -e .
$(PIP) install -e .[docs,test]
If a 2-3 line change addresses this issue I'd be in favor of that.
Alternatively, should the develop
target by default install all ofrak-related dependencies from the source repository (and not from pypi)? I believe @rbs-jacob has suggested such a solution as being advantageous for development as well
No, if would have to be something like
Would that be OK?
What would be the approach for that? One that would probably work is:
|
e2c91c8
to
dcdacdd
Compare
@whyitfor , I implemented an alternative approach where only the P.S. The reson for the other two changes is that:
|
dcdacdd
to
986aaa0
Compare
@ANogin, can you pull master into this for the CI tests? |
@whyitfor , note my earlier caveat:
So reverting this change is not the right thing to do. |
…edballoonsecurity#470) * Support `application/vnd.android.package-archive` mime type for APKs * Add a CHANGELOG entry
…rity#468) * Fix an issue with parallel tests potentially clashing * Run pre-commit
…edballoonsecurity#472) * Use GzipFile python unpacker for speed, fall back on pigz if needed * Add a changelog entry
* Prevent Ghidra autoanalysis on import * Update CHANGELOG.md * Add link to PR in changelog --------- Co-authored-by: Wyatt <[email protected]>
…#478) Co-authored-by: Dan Pesce <[email protected]>
…edballoonsecurity#480) * Make the log file name configurable and include timestamp by default * Add a changelog entry * Fix a help string typo Co-authored-by: Jacob Strieb <[email protected]> --------- Co-authored-by: Jacob Strieb <[email protected]>
…r` (redballoonsecurity#484) * Use temporary output directory for function replacement test * Use pathlib joining for file path
Add SHELL instruction to build_image.py to avoid JSONArgsRecommended warning
make develop
in core into two stages.ofrak_angr
and ofrak_capstone
optional for ofrak_core
tests
@whyitfor in order for things to still [kind of] work with |
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.
This seems like a good incremental compromise.
… optional for `ofrak_core` tests Fixes redballoonsecurity#420
… optional for `ofrak_core` tests (#571) * Fix circular dependencies by making `ofrak_angr` and `ofrak_capstone` optional for `ofrak_core` tests Fixes #420 * Use ofrak-angr-ghidra.yml for all github CI tests * Revert "Use ofrak-angr-ghidra.yml for all github CI tests" This reverts commit a6cd200. * Add `ofrak_angr` to `ofrak_ghidra` per Wyatt's guidance * Revert the workflow file to what it was originally * Do not insist on 100% functional coverage, unless ofrak_angr is there * Typo fix * Should be using the PYTHON variable * Put the commented requirements in, per Wyatt * Always enforce coverage Co-authored-by: Wyatt <[email protected]> * Add changelog entry, ofrak==3.3.0rc1 --------- Co-authored-by: Wyatt <[email protected]> Co-authored-by: Wyatt <[email protected]>
One sentence summary of this PR (This should go in the CHANGELOG!)
Optmizes the build of the --target=development docker containers for the
ofrak_core
"forward" test dependencies onofrak_angr
andofrak_capstone
Link to Related Issue(s)
#419
Please describe the changes in your request.
Split the
make develop
forofrak_core
into two stages, to have the install process agree with the dependency order.Anyone you think should look at this, specifically?
@whyitfor