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 the binary ninja plugin build after the introduction of fmt library into the binary ninja API #122

Merged
merged 1 commit into from
Jan 26, 2024

Conversation

xusheng6
Copy link
Contributor

A while ago, we (vector35) added fmt as a dependency in the binaryninja-api and that is breaking the binexport build. I made two fixes to it so the build can work. The two fixes are:

  1. when cloning the binaryninja-api repo, recursively clone all submodules. This will include the fmt library which is necessary for it to build. The reason that I did not specify this fmt library explicity in the CMake is that will break the build for people trying to build against an earlier version of BN, e.g., the stable 3.5, which is before we introduced the fmt into the binaryninja-api. The tradeoff is several unnecessary submodules will also be cloned, but I think that is a reasonable tradeoff.
  2. I include binaryninjacore.h instead of the binaryninjaapi.h in the binaryninjacore.cc. binaryninjaapi.h references fmt as well as many other irrelevant things for the purpose of building a substitute binaryninjacore, so I included the binaryninjacore.h which has less distractions.

I am testing against the Jordan's binexport build script: https://gist.github.com/psifertex/31d9bc3167eca91e466ebaae4382521c, though I believe it should also fix the build for other script or workflow.

The branch is gonna fail the CI because it does not have access to the workflow secrets. However, it should build just fine if some maintainer triggers the build

@cblichmann
Copy link
Member

Thanks for fixing this.

That said, vendoring the fmt module dependency will make my life a bit harder (we cannot have vendored deps internally, so fmt has to be spun out and maintained sperately in our monorepo).

I'll just accept the PR as is, but IMO there should be a way to not clone extra submodules that are not needed.

@copybara-service copybara-service bot merged commit 3dcf548 into google:main Jan 26, 2024
3 of 6 checks passed
@xusheng6
Copy link
Contributor Author

Thanks for fixing this.

That said, vendoring the fmt module dependency will make my life a bit harder (we cannot have vendored deps internally, so fmt has to be spun out and maintained sperately in our monorepo).

I'll just accept the PR as is, but IMO there should be a way to not clone extra submodules that are not needed.

Thanks for merging the PR, and my apologies for causing any inconvenience on your end.

I totally agree with you that cloning all submodules is not the best solution. I could remove that and explicitly clone the fmt submodule once we release 4.0 and your CI is building against that.

@xusheng6 xusheng6 deleted the fix_binja_build branch May 6, 2024 02:36
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.

2 participants