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

Fix circular dependencies by making ofrak_angr and ofrak_capstone optional for ofrak_core tests #571

Merged

Conversation

ANogin
Copy link
Collaborator

@ANogin ANogin commented Jan 14, 2025

  • I have reviewed the OFRAK contributor guide and attest that this pull request is in accordance with it.
  • I have made or updated a changelog entry for the changes in this pull request.

One sentence summary of this PR (This should go in the CHANGELOG!)
Optmizes the build of the --target=development docker containers by removing the ofrak_core "forward" test dependencies on ofrak_angr and ofrak_capstone

Link to Related Issue(s)
Fixes #419

Please describe the changes in your request.
#226 introduced an unfortunate issue (which I discovered using PR #218 - IMHO we really out to land that one, and use in CI) - when the docker build runs make develop, it would first go to ofrak_core, where the "test": "ofrak_angr"] dependency would cause it to download ofrak_angr from PyPI, then it would go into ofrak_angr and then the make develop there would overwrite the downloaded one with the local one (and similar with ofrak_capstone). With #417, this means that not only the ofrak_angr itself is downloaded from PyPI, but also the angr itself and all its dependencies installed in base.Dockerfile are updated to the versions required by the ofrak_angr from PyPI. Then make develop would put them all back.

A potentially even bigger issue is that the ofrak-ghidra.yaml container would use the ofrak_angr and ofrak_capstone from pypi, not the latest, so the testing in CI would not necessaily be accurately representative of what is actually going on!

This appempts to fix the issues by:

  • Removing the extra dependencies
  • Skipping the test that depend on ofrak_angr and ofrak_capstone when those are not installed
  • Using a new ofrak-angr-ghidra.yml container for all tests

Anyone you think should look at this, specifically?
@whyitfor

Copy link
Contributor

@whyitfor whyitfor left a comment

Choose a reason for hiding this comment

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

@ANogin, please see comments -- once this is done, we can merge this in.

The comments suggest a short-term work-around to this issue.

Longer term, I'd like to have the following true:

  1. ofrak-dev.yml allows a user to build, test, and use the core ofrak components only. We need to remove the ofrak_angr and ofrak_capstone test dependencies here.
  2. ofrak-ghidra.yml allows a user to build, test, and use ofrak with Ghidra.
  3. ofrak-angr.yml allows a users to build, test, and use ofrak with angr.

ofrak_core/setup.py Show resolved Hide resolved
ofrak-angr-ghidra.yml Outdated Show resolved Hide resolved
.github/workflows/test-all.yml Outdated Show resolved Hide resolved
@ANogin
Copy link
Collaborator Author

ANogin commented Jan 15, 2025

  • ofrak-dev.yml allows a user to build, test, and use the core ofrak components only. We need to remove the ofrak_angr and ofrak_capstone test dependencies here.

Isn't that what ofrak-core-dev.yml is for?

@ANogin ANogin requested a review from whyitfor January 15, 2025 16:54
ofrak_core/setup.py Outdated Show resolved Hide resolved
ofrak_core/Makefile Outdated Show resolved Hide resolved
@whyitfor
Copy link
Contributor

  • ofrak-dev.yml allows a user to build, test, and use the core ofrak components only. We need to remove the ofrak_angr and ofrak_capstone test dependencies here.

Isn't that what ofrak-core-dev.yml is for?

Sure, ofrak-core-dev.yml allows a user to build, test, and use the core components only.

@ANogin
Copy link
Collaborator Author

ANogin commented Jan 17, 2025

@whyitfor does this require a Changelog entry?

@whyitfor
Copy link
Contributor

@whyitfor does this require a Changelog entry?

I think so. I'm generally thinking the following:

  1. A Fixed entry here saying that ofrak drops circular test dependencies.
  2. We should bump (in changelog and in setup.py) ofrak's version to 3.3.0rc1. In the changelog, this will just change the Unreleased line. The idea behind this is then we can use these rc versions internally to specify "this is the unreleased version of ofrak I'm using" (this can be helpful for projects that rely on ofrak "master" -- they can specify through versioning that they are on an older version of master.

@rbs-jacob, does this make sense to you?

@rbs-jacob
Copy link
Member

@rbs-jacob, does this make sense to you?

Yup, sounds good to me. I think bumping versions before release is something we probably should have been doing the whole time, but I don't think anyone thought of it until recently.

Copy link
Contributor

@whyitfor whyitfor left a comment

Choose a reason for hiding this comment

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

LGTM

@whyitfor whyitfor merged commit 1c609f8 into redballoonsecurity:master Jan 20, 2025
4 checks passed
@whyitfor whyitfor added this to the 3.3.0 Release milestone Jan 20, 2025
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.

Circular dependency cases a make develop weirdness.
3 participants