fix: Include header files to fix compile errors.#368
fix: Include header files to fix compile errors.#368eyal0 wants to merge 9 commits intogerbv:developfrom
Conversation
If dxf is not found on the system, include it in libgerbv.a. This fixes gerbv#366.
|
This is a draft until the underlying PR #367 is in. |
rampageservices
left a comment
There was a problem hiding this comment.
The math.h/_USE_MATH_DEFINES fix (commit 2) looks good.
A few notes on the dxflib bundling (commit 1):
COMPILE_WARNING_AS_ERROR OFF is a no-op. The project doesn't set -Werror or COMPILE_WARNING_AS_ERROR anywhere, so there's nothing to turn off. This can be dropped.
PUBLIC include dir on the OBJECT library is dead. OBJECT library usage requirements don't propagate through $<TARGET_OBJECTS:>, which is why you had to add the explicit target_include_directories(gerbv PRIVATE ...) in src/CMakeLists.txt. The PUBLIC on dxflib_bundled no longer does anything.
Inconsistent path style. The root CMakeLists.txt changed ${CMAKE_SOURCE_DIR}/thirdparty to the relative thirdparty, but src/CMakeLists.txt uses ${CMAKE_SOURCE_DIR}/thirdparty. Both work, but the inconsistency is unnecessary.
Simpler alternative: skip the OBJECT library entirely. Since the only goal is embedding dxflib symbols into libgerbv.a, you can just add the sources directly to the gerbv target:
# Root CMakeLists.txt — replace the add_library block:
if(NOT DXF_FOUND)
set(DXFLIB_BUNDLED_SOURCES
${CMAKE_SOURCE_DIR}/thirdparty/dxflib/dl_dxf.cpp
${CMAKE_SOURCE_DIR}/thirdparty/dxflib/dl_writer_ascii.cpp)
set(DXF_FOUND TRUE)
set(DXFLIB_BUNDLED TRUE)
endif()
# src/CMakeLists.txt — in the DXF_FOUND block:
if(DXFLIB_BUNDLED)
target_sources(gerbv PRIVATE ${DXFLIB_BUNDLED_SOURCES})
target_include_directories(gerbv PRIVATE ${CMAKE_SOURCE_DIR}/thirdparty)
endif()This eliminates the dxflib_bundled target, the OBJECT library, the dead PUBLIC include propagation, and the unnecessary COMPILE_WARNING_AS_ERROR OFF — while achieving the same result.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
|
Thanks for identifying the root cause here @eyal0 — I've put up #369 as a simplified alternative that compiles the bundled dxflib sources directly into the I've credited you as co-author on both commits. |
I think that this is not true. ci.yml uses a preset for cmake: https://github.com/gerbv/gerbv/blob/develop/.github/workflows/ci.yaml#L65 That preset is here: https://github.com/gerbv/gerbv/blob/develop/CMakePresets.json#L4 It loads this: https://github.com/gerbv/gerbv/blob/develop/cmake/preset/linux-gnu-gcc.json#L3 Which loads the base.json: https://github.com/gerbv/gerbv/blob/develop/cmake/preset/base.json#L10 |
|
I do prefer your simple alternative. Go for it! |
On some systems, like MVC++ and strict ANSI/C standards, M_PI and others are not defined.
|
You were right about Updated #369 to handle it: the bundled dxflib sources now get Also moved the |
|
Heads up — spe-ciellt opened #374 to restructure all thirdparty library handling into I've closed #369 in favor of that approach and left review comments on #374 noting the fixes that still need to go in (dxflib embedding, warning suppression, You may want to close this one too and coordinate with #374 directly, since the OBJECT library approach here would conflict with the new directory layout. |
|
Leaving this here for reference: This branch of gerbv successfully compiles on both ubuntu and mingw32 windows. Also, the libgerbv.a created on both platforms works successfully in pcb2gcode. The necessary changes are:
It might be possible to get M_PI et al in another way but |
The windows compilation seems more sensitive to missing header files than linux compilation.