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

build: do not install bundled Rizin in separate prefix #3254

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

ret2libc
Copy link
Member

Your checklist for this pull request

Detailed description

I was looking at #3229 (comment) and I wanted to see how bundled Rizin worked. So I tried to build Cutter with bundled rizin and install everything in a /tmp/cutter-install directory to see what was actually installed. It turned out that I found a /tmp/cutter-install/Users/ret2libc/cutter/build/Rizin-prefix directory being installed. Not only that, but the rizin files were both there and in the actual cutter prefix, because they are "manually" copied here.

The solution I'm proposing is to avoid the install step of rizin and just install Rizin in the build step, installing the data in a "temporary" location in the build directory. Then, at install time, the instructions in BundledRizin.cmake will copy the right files from the build dir into the install location. Moreover, rizin is compiled with the prefix being the same as cutter, so that rz_userconf.h will contain the right prefix and all the various rizin-specific directories will be correctly loaded.

Hope it makes sense :)

Test plan (required)

$ git clone cutter
$ cmake -B build
$ cmake --build build
$ DESTDIR=/tmp/cutter-install cmake --install build

In /tmp/cutter-install there should be only the /usr/local/... dir and not other dirs for the rizin-specific prefix.

Copy link
Member

@XVilka XVilka left a comment

Choose a reason for hiding this comment

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

It makes sense if this approach works on all supported platforms.

It breaks Windows though:

'DESTDIR' is not recognized as an internal or external command,
operable program or batch file.
ninja: build stopped: subcommand failed.
Command exited with code 1

'DESTDIR' is not recognized as an internal or external command,

operable program or batch file.

ninja: build stopped: subcommand failed.
[1/189] Performing build step for 'Rizin-Bundled'
FAILED: Rizin-Bundled-prefix/src/Rizin-Bundled-stamp/Rizin-Bundled-build D:/a/cutter/cutter/build/Rizin-Bundled-prefix/src/Rizin-Bundled-stamp/Rizin-Bundled-build 
cmd.exe /C "cd /D D:\a\cutter\cutter\build\Rizin-Bundled-prefix\src\Rizin-Bundled-build && C:\hostedtoolcache\windows\Python\3.7.9\x64\Scripts\ninja.exe && DESTDIR=D:/a/cutter/cutter/build C:/hostedtoolcache/windows/Python/3.7.9/x64/Scripts/ninja.exe install"
[1/1] Generating gittip_file with a custom command

'DESTDIR' is not recognized as an internal or external command,

operable program or batch file.

[2/189] Performing update step for 'rz-ghidra'
ninja: build stopped: subcommand failed.

@ret2libc
Copy link
Member Author

It makes sense if this approach works on all supported platforms.

It breaks Windows though:

'DESTDIR' is not recognized as an internal or external command,
operable program or batch file.
ninja: build stopped: subcommand failed.
Command exited with code 1

'DESTDIR' is not recognized as an internal or external command,

operable program or batch file.

ninja: build stopped: subcommand failed.
[1/189] Performing build step for 'Rizin-Bundled'
FAILED: Rizin-Bundled-prefix/src/Rizin-Bundled-stamp/Rizin-Bundled-build D:/a/cutter/cutter/build/Rizin-Bundled-prefix/src/Rizin-Bundled-stamp/Rizin-Bundled-build 
cmd.exe /C "cd /D D:\a\cutter\cutter\build\Rizin-Bundled-prefix\src\Rizin-Bundled-build && C:\hostedtoolcache\windows\Python\3.7.9\x64\Scripts\ninja.exe && DESTDIR=D:/a/cutter/cutter/build C:/hostedtoolcache/windows/Python/3.7.9/x64/Scripts/ninja.exe install"
[1/1] Generating gittip_file with a custom command

'DESTDIR' is not recognized as an internal or external command,

operable program or batch file.

[2/189] Performing update step for 'rz-ghidra'
ninja: build stopped: subcommand failed.

I bet there's a way to make it work on windows too, i'll have a look.

@@ -39,20 +39,20 @@ endif()
ExternalProject_Add(Rizin-Bundled
SOURCE_DIR "${RIZIN_SOURCE_DIR}"
CONFIGURE_COMMAND "${MESON}" "<SOURCE_DIR>" ${MESON_OPTIONS} && "${MESON}" configure ${MESON_OPTIONS} --buildtype "$<$<CONFIG:Debug>:debug>$<$<NOT:$<CONFIG:Debug>>:release>"
BUILD_COMMAND "${NINJA}"
BUILD_COMMAND "${NINJA}" && "DESTDIR=${RIZIN_INSTALL_DIR}" "${NINJA}" install
Copy link
Member

@ITAYC0HEN ITAYC0HEN Oct 27, 2023

Choose a reason for hiding this comment

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

Won't work on Windows (with this syntax)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I need to fix it. I guess there's a BUILD_ENV or something in Cmake, i need to check doc.

@karliss
Copy link
Member

karliss commented Oct 30, 2023

There are couple of different usecases with different needs:

  1. Development builds
  2. Linux distro packaging builds
  3. Semi permanent user builds, installed to some random prefix not necessarily at system level
  4. Self contained Cutter packaging builds
    4.1) Appimage
    4.2) Windows Cutter release package
    4.3) macOS Cutter release package

Case 2) will probably want to compile rizin separately anyway (with exception of few pentesting tools oriented distros, maintained by people lacking the basic understanding how package building process for their base distro work). So for that bundled rizin option isn't too important.

From what I remember case 1) was a significant reasons why bundled rizin option even exists.

Case 4) somehow works with the old version. No idea how much it is due passing special cmake arguments for those builds, and how much it's deployqt doing magic while moving the so libs.

From what I remember only case where the old approach didn't work, at least not out of the box was case 3).

I assume that this PR makes case 3) somewhat simpler. At the same time it breaks case 1) (just tested it really breaks it as I was worried). No idea on the influence on cases 2) and 4).

I understand that taking into account my recent participation level, my opinion doesn't mean too much, but I would really like to preserve the case 1) working without having to pass any special arguments to cmake.

That is doing basic steps of:

  • do a checkout with submodules
  • mkdir build; cd build
  • cmake ..
  • make

And no install step , should produce a running development build. This has multiple benefits:

  • Simplify the onboarding exprience of new contributors. Any addition step or project specific argument, increases the chance of person giving up early. With all the other open source I have contributed or simply compiled for myself, I highly appreciate when standard build process works.
  • Better integration with various IDEs, if the default build can be directly run. It means that IDE builtin commands for simply runing, compiling and runing, debuging, building and running in special mode (ie with profiler or sanitizer) just works without additional manual configuration. If project requires installing, before the executables properly run, the chance of requiring manual config or some of those convenience features not working increases.
  • Having to install, slows down the edit->test cycle. With properly functioning incremental builds editing single file and pressing run, shouldn't cause much more than compiling that single file, relinking executable and launching it. From my experience the installation process isn't as well integrated with incremental builds, especially if the project contains custom install commands that are blackbox from the perspective of build system, thus causing redundant work and making the build much slower.

From my experience when I was doing some patches for rizin, I was quite annoyed that the builds didn't fully work without installation.

Similar for the case 2) the amount of special arguments required should be kept minimal (besides feature configuriation, and common cmake arguments that packagers would use for most other CMake builds)

With regards to rizin not working when not installed from what I remember and also doing quick test builds of this merge request, the problem wasn't as much that it didn't work at all, but that it failed to find some nonexecutable resources - all the sdb files and stuff like that. It probably possible to make it run, by passing some argument or enviornment variable to override the resource search path, but I don't consider it an acceptable solution. See the previous comment about being able to build and run without additional steps.

If there are any build arguments passed either during configuration or install steps by BundleRizin cmake script so that both usescases 1) and 3) work without additional manual configuration that would be the best. Maybe they are already there, and I am just not sufficiently familiar with how rizin build process works.

One more requirement that I would like to be satisfied is that in useceas 3) and 4) the bundled rizin executables should also work by themselves, without special arguments or environment variables. That is the whole setup can't depend On Cutter executable passing in resource location override to the rizin libs, which wouldn't happen when running the rizin CLI tools. Unless those overrides are only needed when running as libs within Cutter.

@ret2libc
Copy link
Member Author

@karliss thanks for the reply! Your opinion is ofc always well accepted :) I will try to keep the case 1 as you mentioned.

@karliss
Copy link
Member

karliss commented Nov 2, 2023

If you want the usecase 3) (linux only) one way to achieve it currently with no modifications to build scripts is like this

cmake -DCMAKE_INSTALL_PREFIX=/some/random/prefix/ -DCMAKE_INSTALL_RPATH="\$ORIGIN/../lib"  -DCUTTER_ENABLE_PACKAGING=ON ..

Explanation why it's necessary and works:

  • CMAKE_INSTALL_PREFIX -> one of the usual methods for specifying install prefix when building something with cmake
  • CMAKE_INSTALL_RPATH -> necessary so that cutter can locate rizin libs using relative path
  • CUTTER_ENABLE_PACKAGING -> will build rizin with -Dportable=true thus allowing it to find it's resources even though during build it's installed in different directory

Usage of CUTTER_ENABLE_PACKAGING is a bit of hack, it's meant to be used for usecase 4) but currently on Linux it only changes -Dportable. On other platforms (and possibly future versions of Cutter of Linux) it does a few other things which you wouldn't want for 3). But for now it works.

From what I can tell only thing which currently doesn't work in such build is translations.

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.

5 participants