jeremy-rifkin / cpptrace

Simple, portable, and self-contained stacktrace library for C++11 and newer
MIT License
701 stars 75 forks source link

Use the new libdwarf behavior to compile with zstd.a and zlib.a #81

Closed flagarde closed 8 months ago

flagarde commented 9 months ago

@jeremy-rifkin Hi, based on your https://github.com/davea42/libdwarf-code/issues/216 I have played a bit in order to do a repo to generate libdwarf.a using FetContent(zlib) FetContent(ZSTD) and FetContent(libdwarf). It is very hacky but should work.

You can have a look if you want and try to use it if you think it is useful.

https://github.com/flagarde/libdwarf-build

Still need some polishing but it should make the work.

jeremy-rifkin commented 9 months ago

Thank you @flagarde! I'll play around with this on dev

jeremy-rifkin commented 9 months ago

I'm running into an include error with zstd:

/mnt/c/Users/rifkin/home/projects/cpptrace/build/_deps/libdwarf-src/src/lib/libdwarf/dwarf_init_finish.c:60:10: fatal error: zstd.h: No such file or directory
   60 | #include "zstd.h"
      |          ^~~~~~~~

This is my current cmake (only doing zstd+libdwarf at the moment and relying on the system for zlib)

    include(FetchContent)
    cmake_policy(SET CMP0074 NEW)
    FetchContent_Declare(
        zstd
        GIT_REPOSITORY https://github.com/facebook/zstd.git
        GIT_TAG 63779c798237346c2b245c546c40b72a5a5913fe
        SOURCE_SUBDIR build/cmake
    )
    FetchContent_GetProperties(zstd)
    if(NOT zstd_POPULATED)
      FetchContent_Populate(zstd)
      set(ZSTD_BUILD_PROGRAMS OFF)
      set(ZSTD_BUILD_CONTRIB OFF)
      set(ZSTD_BUILD_TESTS OFF)
      set(ZSTD_BUILD_STATIC ON)
      set(ZSTD_BUILD_SHARED OFF)
      set(ZSTD_LEGACY_SUPPORT OFF)
      add_subdirectory("${zstd_SOURCE_DIR}/build/cmake" "${zstd_BINARY_DIR}")
    endif()

    if(TARGET libzstd_static)
      add_library(ZSTD::ZSTD ALIAS libzstd_static)
      set(ZSTD_FOUND TRUE)
    endif()

    set(CMAKE_POLICY_DEFAULT_CMP0077 NEW)
    # set(PIC_ALWAYS TRUE)
    # set(BUILD_DWARFDUMP FALSE)
    include(FetchContent)
    FetchContent_Declare(
      libdwarf
      GIT_REPOSITORY https://github.com/davea42/libdwarf-code.git
      GIT_TAG c0cfba34ec80996426b5be2523f6447a2c9b7b39 # v0.9.0 + mach-o changes
      GIT_SHALLOW 1
    )
    FetchContent_GetProperties(libdwarf)
    if(NOT libdwarf_POPULATED)
      set(PIC_ALWAYS TRUE)
      set(BUILD_DWARFDUMP FALSE)
      FetchContent_Populate(libdwarf)
      add_subdirectory("${libdwarf_SOURCE_DIR}" "${libdwarf_BINARY_DIR}")
    endif()
jeremy-rifkin commented 9 months ago

I'm able to get it to build if I add

      target_include_directories(
        dwarf
        PRIVATE
        ${zstd_SOURCE_DIR}/lib
      )

Zstd mentions this is needed on mac and windows, though here I'm compiling on linux. If you have any ideas about this I'd be interested but I can move forward with this though!

flagarde commented 9 months ago

Weird bit not impossible, the cmake is not officially made and seems a contributor addition. I guess on my system th zstd.h was in PATH or standard looking path so didn't trigger the error.

Anyway the addition is small an not so hacky.

Do you plan to add zlib too. Would be nice to have zlib-ng possibility and the are working on a nice config file these days so it would be even easier. It is supposed to be fully compatible but have better performances than traditional zlib and to have a static lib would be great

jeremy-rifkin commented 9 months ago

Good to know that the cmake isn't official. I agree the addition should be ok.

For now I was planning on just doing zstd and relying on the system's zlib. However, I'm not opposed to adding some flag to grab it through fetchcontent (or elsewhere).

flagarde commented 9 months ago

Sure zlib is quite common and it is nice to rely on system for this but it would be great to have an option to bypass it

flagarde commented 9 months ago

Good to know that the cmake isn't official. I agree the addition should be ok.

For now I was planning on just doing zstd and relying on the system's zlib. However, I'm not opposed to adding some flag to grab it through fetchcontent (or elsewhere).

Well it is on repo but in build so maybe it is official but not considered as main build system

jeremy-rifkin commented 9 months ago

Gotcha. I'm working on wrapping up some mach-o stuff at the moment and I'll return to this before the next release :)

jeremy-rifkin commented 9 months ago

@flagarde I've ran into a bit of an oddity while trying to get the aforementioned mach-o changes merged in. In d94a58d416a7d77dd6f1e9ca650c82fcfea48237 all builds and tests are passing, however when I have another script using cpptrace from find_package I get errors from libdwarf

-- Configuring done (0.2s)
CMake Error at /usr/local/lib/cmake/libdwarf/libdwarf-targets.cmake:61 (set_target_properties):
  The link interface of target "libdwarf::dwarf" contains:

    ZLIB::ZLIB

  but the target was not found.  Possible reasons include:

    * There is a typo in the target name.
    * A find_package call is missing for an IMPORTED target.
    * An ALIAS target is missing.

Call Stack (most recent call first):
  /usr/local/lib/cmake/libdwarf/libdwarf-config.cmake:2 (include)
  /usr/local/share/cmake-3.28/Modules/CMakeFindDependencyMacro.cmake:76 (find_package)
  /usr/local/lib/cmake/cpptrace/cpptrace-config.cmake:7 (find_dependency)
  CMakeLists.txt:7 (find_package)

https://github.com/jeremy-rifkin/cpptrace/actions/runs/7664134879 https://github.com/jeremy-rifkin/cpptrace/actions/runs/7664134877

I'm wondering if libdwarf's cmake install might be missing something. Maybe libdwarf-config.cmake should include FindZLIB? I haven't been able to reproduce this locally yet, only on github's runners. I'd be interested if you have any ideas.

flagarde commented 9 months ago

Yes I guess libdwarf config file is not written properly.

I plan to write some PR to libdwarf for this but it's not yet done

I guess Zlib is a .so right? Then yes I guess libdwarf should import it in config file

flagarde commented 9 months ago

@jeremy-rifkin Far from perfect but maybe you could have a try on my repo here https://github.com/flagarde/libdwarf-code I made some changes. If it works I could try to make them accepted for upstream. Basically I made libdwarfConfig to make it find what it needs. Not sure if it's working fine with .a

jeremy-rifkin commented 9 months ago

Thank you @flagarde! I gave it a try on CI though I ran into a little error

CMake Error at _deps/libdwarf-build/src/lib/libdwarf/cmake_install.cmake:102 (file):
  file INSTALL cannot find
  "/home/runner/work/cpptrace/cpptrace/cmake/FindZSTD.cmake": No such file or
  directory.
Call Stack (most recent call first):
  _deps/libdwarf-build/cmake_install.cmake:47 (include)
  cmake_install.cmake:47 (include)

https://github.com/jeremy-rifkin/cpptrace/actions/runs/7680658705/job/20932962789?pr=82

It looks like ${CMAKE_SOURCE_DIR} is using cpptrace's source directory rather than libdwarf's

flagarde commented 9 months ago

Oh yes my bad. But this is quite easy to solve. Maybe a PROJECT_SOURCE_DIR 🤔

flagarde commented 9 months ago

@jeremy-rifkin I pushed the change it should be ok now

jeremy-rifkin commented 9 months ago

Thanks so much! Those changes worked perfectly and CI is passing :)

jeremy-rifkin commented 9 months ago

I have another quick question, what's the new canonical target for libdwarf? Is it libdwarf::libdwarf or libdwarf::dwarf?

flagarde commented 9 months ago

I have another quick question, what's the new canonical target for libdwarf? Is it libdwarf::libdwarf or libdwarf::dwarf?

I don't remember what I have written. But we can ask to the ower of the lib what he prefers.

It seems it's dwarf but i should maybe change this. I will ask the owner. But you should be able to find add_library(libdwarf::dwarf-static ALIAS dwarf) or shared too.

Some polishing should maybe done with naming.

jeremy-rifkin commented 9 months ago

@flagarde Pulling your changes into the workflows for this repo I'm seeing some build failures on windows: https://github.com/jeremy-rifkin/cpptrace/actions/runs/7705960162/job/21000722775 https://github.com/jeremy-rifkin/cpptrace/actions/runs/7705960161/job/21000720841

CMake Error at C:/Program Files/CMake/share/cmake-3.27/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
  Could NOT find ZSTD (missing: ZSTD_LIBRARIES) (found version "1.5.5")
Call Stack (most recent call first):
  C:/Program Files/CMake/share/cmake-3.27/Modules/FindPackageHandleStandardArgs.cmake:600 (_FPHSA_FAILURE_MESSAGE)
  C:/Program Files (x86)/libdwarf/lib/cmake/libdwarf/FindZSTD.cmake:56 (find_package_handle_standard_args)
  C:/Program Files/CMake/share/cmake-3.27/Modules/CMakeFindDependencyMacro.cmake:76 (find_package)
  C:/Program Files (x86)/libdwarf/lib/cmake/libdwarf/libdwarfConfig.cmake:34 (find_dependency)
  CMakeLists.txt:350 (find_package)

Zstd is also not found during libdwarf's configuration, and I thought the config wouldn't attempt to locate it in that case

As a side note, I've merged the mach-o stuff I was working on and hope to do a cpptrace release in the coming days, though it'll depend on upstream changes from libdwarf.

flagarde commented 9 months ago

@jeremy-rifkin I think it is because the library is not in the search paths. A very hack solution maybe could be to add an hint to your libdward binding repo how to find it; a ROOT_ZSTD or maybe to play with the RPATH variables in CMake.

I'm not really big expert on this find_package stuffs. I would like to find a way to detect if found_libraries are static or shared because if they are static than libdwarf found package doesn't need to look for them anymore as it is already compiled with the necessary part. But I didn't find any CMake way to do this and I'm not sure to know how to do this in a cross platform way.

jeremy-rifkin commented 9 months ago

Thanks, I might try to fiddle around with some lidwarf changes and see about making a pr

flagarde commented 9 months ago

@jeremy-rifkin Maybe this strange thing https://github.com/facebook/zstd/blob/a7b4dafa0323441f485c2387fd1904da67f386a1/build/cmake/lib/CMakeLists.txt#L165

flagarde commented 9 months ago

@jeremy-rifkin I found a bug in my Config file I think it should be

set(CMAKE_MODULE_PATH_OLD "${CMAKE_MODULE_PATH}")
jeremy-rifkin commented 9 months ago

Ope, thanks for noticing. Do you want to PR upstream?

jeremy-rifkin commented 9 months ago

I am putting together a PR with changes needed for conan so I'll go ahead and include it in there

jeremy-rifkin commented 8 months ago

0.4 has been released