google / binexport

Export disassemblies into Protocol Buffers
Apache License 2.0
1.03k stars 197 forks source link

Better support for latest binary ninja builds? #108

Open psifertex opened 1 year ago

psifertex commented 1 year ago

Rather than manually updating commit hashes would you consider a PR for something like this?

diff --git a/cmake/BinExportDeps.cmake b/cmake/BinExportDeps.cmake
index 8832d98..757adb9 100644
--- a/cmake/BinExportDeps.cmake
+++ b/cmake/BinExportDeps.cmake
@@ -87,14 +87,26 @@ find_package(Protobuf 3.14 REQUIRED) # Make protobuf_generate_cpp available

 # Binary Ninja API
 if(BINEXPORT_ENABLE_BINARYNINJA)
-  if(BINEXPORT_BINARYNINJA_CHANNEL STREQUAL "stable")
-    set(_binexport_binaryninjacore_suffix "_stable")
-    set(_binexport_binaryninja_git_tag
-        "master") # 2022-10-18
+  if(BINEXPORT_BINARYNINJA_LATEST)
+    if(BINEXPORT_BINARYNINJA_CHANNEL STREQUAL "stable")
+      set(_binexport_binaryninjacore_suffix "_stable")
+      set(_binexport_binaryninja_git_tag
+          "master")
+    else()
+      set(_binexport_binaryninjacore_suffix "")
+      set(_binexport_binaryninja_git_tag
+          "dev")
+    endif()
   else()
-    set(_binexport_binaryninjacore_suffix "")
-    set(_binexport_binaryninja_git_tag
-        "b160aa0bac77a45bd7c4f39c6fe0263059239d4b") # 2022-10-20
+    if(BINEXPORT_BINARYNINJA_CHANNEL STREQUAL "stable")
+      set(_binexport_binaryninjacore_suffix "_stable")
+      set(_binexport_binaryninja_git_tag
+          "824aa7f7fc88713e74932c6d3230a6fb2d29df97") # 2022-10-18
+    else()
+      set(_binexport_binaryninjacore_suffix "")
+      set(_binexport_binaryninja_git_tag
+          "b160aa0bac77a45bd7c4f39c6fe0263059239d4b") # 2022-10-20
+    endif()
   endif()
   FetchContent_Declare(binaryninjaapi
     GIT_REPOSITORY https://github.com/Vector35/binaryninja-api.git

I realize you might not want the default to automatically pull from a named branch but it makes my life easier to have it as an option since I'm almost always running on the latest dev.

psifertex commented 1 year ago

Note that after I made this change locally I also had to make sure I updated the generated headers which made me wonder -- since the repo is being downloaded/checked out anyway, why not generate that inline during the cmake process?

cblichmann commented 1 year ago

I could've sworn I already answered on this issue. I'll be happy to accept a PR, if BINEXPORT_BINARYNINJA_LATEST defaults to OFF.

[...] since the repo is being downloaded/checked out anyway, why not generate that inline during the cmake process?

There is nothing stopping us to do so, but then we need something that works the same on Windows, Linux and macOS. And so far, it was just easier to regenerate the headers on Linux whenever I update Binary Ninja in our internal third_party/ sub-tree.

psifertex commented 1 year ago

That makes sense. Given how sensitive it currently makes the update process to our API changes, I'm going to at a minimum submit some improved documentation in the readme that would help people be aware of the process, and if I'm lucky and have any good ideas something on automating the process itself.

Random brainstorm: Since BN itself comes with a type import/export ultimately based on clang, I wonder if we can't implement it as a BN plugin. The only downside would be non-commercial licenses wouldn't be able to headlessly automate that step, but at least everyone building would inherently already have the tools required since if they're building for BN they presumably have access to BN! 🤔

cblichmann commented 1 year ago

type import/export ultimately based on clang

For various reasons I cannot run commercial Binary Ninja as part of our build infra.

psifertex commented 1 year ago

Even if you can't, users who want to clone/build it could use it if it's available. clang-format isn't necessarily going to be on their box, but BN almost certainly is if they're enabling those settings.

cblichmann commented 1 year ago

That's true. So basically, we then should have a new CMake option that enables to regenerate the "bindings"/header files. It should default to ON, if CMake found an actual Binary Ninja install.

psifertex commented 1 year ago

Yup, that's what I'm thinking. It would simplify the process for people who want to do their own builds to track dev for example.

I'll get with some of my co-workers who are better at cmake than me and see if we can put together a PR for that.