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

Better support for latest binary ninja builds? #108

Open
psifertex opened this issue Feb 7, 2023 · 7 comments
Open

Better support for latest binary ninja builds? #108

psifertex opened this issue Feb 7, 2023 · 7 comments

Comments

@psifertex
Copy link

psifertex commented Feb 7, 2023

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
Copy link
Author

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
Copy link
Member

cblichmann commented Feb 8, 2023

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
Copy link
Author

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
Copy link
Member

type import/export ultimately based on clang

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

@psifertex
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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.

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

No branches or pull requests

2 participants