search

radareorg / r2retdec

RetDec plugin for Radare2
https://retdec.com/
MIT License
124 stars 24 forks source link

Multiple issues while building. #19

Closed KOLANICH closed 4 years ago

KOLANICH commented 4 years ago
  1. This uses FetchContent and rebuilds retdec. It is completely inacceptable, it should link prebuilt retdec libs instead.
  2. Arguments passed in some RapidJSON calls are wrong. Had to fix that manually.
  3. RapidJSON is a header-only lib. It should not be mentioned in target_link_libraries.
  4. You duplicate c++17-enabling flags. First you opportunistically enable them using CMake CXX_STANDARD, and then using explicit gcc-specific flag (i.e. unsuitable for Visual Studio). You should leaveonly CXX_STANDARD and maybe set it to 20 (it is the highest version used, not the lowest).
xkubov commented 4 years ago

Hi, thanks for the notes.

  1. Currently, I am working on changes for next release and they are included in the retdec-lib branch. As the name of the branch suggests plugin is being rewritten so that RetDec is linked to the plugin. I think that you are right and I will also provide option for RetDec not to be fetched but searched in standard places first.

  2. If you fixed something you think is wrong then feel free to open a new pull request.

  3. The reason why is this rapidjson mentioned in target_link_libraries is that it is only an interface library that is not being built. It servers only as an interface for the path of header files. But as I have removed rapidjson from the repository (it is included in retdec::config) I don't really think it is needed now. Thanks for noticing, I'll take a look at this.

  4. I agree that CMakeListst.txt needs and is going to get a little cleaning. This duplication happens, however, only on macOS/Linux (From MakeLists.txt: elseif(UNIX)) and Windows should not be affected.

xkubov commented 4 years ago

Problems mentioned in this issue are now solved/merged into master. If you want to use the RetDec library that is available somewhere in the system, you can use -DBUILD_BUNDLED_RETDEC=off. I am closing this issue for now.