Skip to content

build(cmake): use add_compile_options not C_FLAGS - #364

Merged
mereacre merged 4 commits into
mainfrom
build/fix-CMAKE_C_FLAGS
Dec 12, 2022
Merged

build(cmake): use add_compile_options not C_FLAGS#364
mereacre merged 4 commits into
mainfrom
build/fix-CMAKE_C_FLAGS

Conversation

@aloisklink

Copy link
Copy Markdown
Contributor

CMake does not recommend modifying CMAKE_C_FLAGS directly if we can help it. Instead, we should be using add_compile_options().

This also means that we no longer overwrite CMAKE_C_FLAGS, which will mean that edgesec works better with other build systems (e.g. pdebuild).

Depends on #360 and #363 (since we no longer overwrite CMAKE_C_FLAGS, some of the debian build's C_FLAGS cause errors that need to be fixed).

CMake does not recommend modifying CMAKE_C_FLAGS directly if we can
help it. Instead, we should be using add_compile_options().

This also means that we no longer overwrite CMAKE_C_FLAGS, which will
mean that `edgesec` works better with other build systems
(e.g. pdebuild).
@codecov

codecov Bot commented Dec 9, 2022

Copy link
Copy Markdown

Codecov Report

Merging #364 (99aa81d) into main (8316437) will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #364      +/-   ##
==========================================
+ Coverage   51.77%   51.80%   +0.02%     
==========================================
  Files         139      139              
  Lines       19318    19318              
==========================================
+ Hits        10002    10007       +5     
+ Misses       9316     9311       -5     
Impacted Files Coverage Δ
tests/test_edgesec.c 94.53% <0.00%> (-0.79%) ⬇️
src/ap/ap_service.c 50.85% <0.00%> (+3.42%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@mereacre
mereacre self-requested a review December 9, 2022 16:09

@mereacre mereacre left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

From cmake docs:
The preprocessor definitions are added to the COMPILE_DEFINITIONS directory property for the current CMakeLists file. They are also added to the COMPILE_DEFINITIONS target property for each target in the current CMakeLists file.

In the cheri-pure branch I add compiler flags CMAKE_C_FLAGS in cheri-cross.cmake file located in CMakeToolchains. Now the question is will they be applied to the entire project?

@aloisklink
aloisklink marked this pull request as ready for review December 12, 2022 10:06
@mereacre
mereacre merged commit 400add4 into main Dec 12, 2022
@mereacre
mereacre deleted the build/fix-CMAKE_C_FLAGS branch December 12, 2022 10:08
@aloisklink

Copy link
Copy Markdown
Contributor Author

In the cheri-pure branch I add compiler flags CMAKE_C_FLAGS in cheri-cross.cmake file located in CMakeToolchains. Now the question is will they be applied to the entire project?

Yes, CMake Toolchain files are applied globally. The add_compile_options() will only apply to targets declared after the add_compile_options() call, but this PR adds those changes before everything is declared, so that's fine.

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.

2 participants