Repository navigation
Conversation
…the Fortran API is optional and the compilation flags for warnings can be set conditionally
benegee
left a comment
benegee
left a comment
There was a problem hiding this comment.
Thanks a lot for your contribution @Niklas997 !
I like the added options! About the warning I am not sure yet.
benegee
left a comment
benegee
left a comment
There was a problem hiding this comment.
Oh no, I just realized one of my comments was still pending in github. @Niklas997
| or debug code. | ||
| - By default the Fortran-Interface is built; it may be excluded with `DLIBTRIXI_BUILD_FORTRAN_API=OFF`. | ||
| - Optional compiler options `-Wall -Wextra` are specified with `-DLIBTRIXI_ENABLE_WARNINGS=ON`. | ||
| - Optional compiler option `-Werror` is specified with `-DLIBTRIXI_ENABLE_WERROR=ON`. |
There was a problem hiding this comment.
So far, we have been very strict with respect to warning, because we could. I would thus like to keep it this way, or at least have them on by default.
Regarding the Julia 1.12 issue: #259 (comment)
Currently I would set -DCMAKE_C_FLAGS="-Wno-error=unused-parameter" when running cmake.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #299 +/- ##
=======================================
Coverage 96.11% 96.11%
=======================================
Files 25 25
Lines 1260 1260
Branches 74 74
=======================================
Hits 1211 1211
Misses 45 45
Partials 4 4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| - Optional specification of build type sets some default compiler options for optimized | ||
| or debug code. | ||
| - Building with t8code support is optional. It requires to pass | ||
| - By default the Fortran-Interface is built; it may be excluded with `DLIBTRIXI_BUILD_FORTRAN_API=OFF`. |
There was a problem hiding this comment.
| - By default the Fortran-Interface is built; it may be excluded with `DLIBTRIXI_BUILD_FORTRAN_API=OFF`. | |
| - By default the Fortran-Interface is built; it may be excluded with `-DLIBTRIXI_BUILD_FORTRAN_API=OFF`. |
| # Include directories | ||
| target_include_directories( ${PROJECT_NAME} PRIVATE src ${JULIA_INCLUDE_DIRS} ) |
There was a problem hiding this comment.
| # Include directories | |
| target_include_directories( ${PROJECT_NAME} PRIVATE src ${JULIA_INCLUDE_DIRS} ) | |
| # Include directories | |
| # PRIVATE: Do not include internal directory in projects building against libtrixi | |
| # SYSTEM: Disable warnings/error from upstream libraries | |
| target_include_directories( ${PROJECT_NAME} PRIVATE src ) | |
| target_include_directories( ${PROJECT_NAME} SYSTEM PRIVATE ${JULIA_INCLUDE_DIRS} ) |
Instead of disabling warnings/errors, I recommend to just mark the Julia include dirs as "SYSTEM" includes such that GCC/Clang suppress warnings on them (see also the CMake docs.
If this works as expected, I'd also recommend reverting the option of disabling warnings/errors: In practice this leads too often to issues that remain unfixed, and if the "SYSTEM" switch suppresses the warning, we can remove a lot of code complexity from the build system again
There was a problem hiding this comment.
Checked in #319 where I also switched to Julia 1.12, which caused the warnings. It seems to work!
| or debug code. | ||
| - By default the Fortran-Interface is built; it may be excluded with `DLIBTRIXI_BUILD_FORTRAN_API=OFF`. | ||
| - Optional compiler options `-Wall -Wextra` are specified with `-DLIBTRIXI_ENABLE_WARNINGS=ON`. | ||
| - Optional compiler option `-Werror` is specified with `-DLIBTRIXI_ENABLE_WERROR=ON`. |
| # Enable C and Fortran | ||
| enable_language(C Fortran) | ||
| # Additional configuration options | ||
| option( LIBTRIXI_BUILD_FORTRAN_API "Enable the build of the Fortran interface" ON ) |
There was a problem hiding this comment.
If this is added as an option, you could think about adding also a test - or at least a smoke test - to ensure that builds still succeed with the Fortran interface turned off (I don't think running tests is necessary). It's not super high priority though, so you might also skip the tests 🤷♂️
| add_library ( ${PROJECT_NAME}_tls OBJECT | ||
| src/tls.c | ||
| ) | ||
| target_include_directories( ${PROJECT_NAME}_tls PRIVATE ${JULIA_INCLUDE_DIRS} ) |
There was a problem hiding this comment.
| target_include_directories( ${PROJECT_NAME}_tls PRIVATE ${JULIA_INCLUDE_DIRS} ) | |
| target_include_directories( ${PROJECT_NAME}_tls SYSTEM PRIVATE ${JULIA_INCLUDE_DIRS} )) |
As above
|
@Niklas997 We just merged #319, which allows to disable the Fortran tests. Regarding the warning we introduced cmake's |
4 participants
Thank you for developing libtrixi! I wanted to use it with t8code and only needed the C-interface, therefore, I added some CMake-Options to exclude building the Fortran-interface. Additionally, the
-Wall -Wextra -Werroroptions have been set obligatory, which has led to errors for me (Ubuntu 24.04. (WSL2), GCC-Version: 13.3.0) due to some unused parameters in the garbage-collection-interface, e.g. "~/.julia/juliaup/julia-1.12.2+0.x64.linux.gnu/include/julia/gc-interface.h:243:47: warning: unused parameter ‘parent’ [-Wunused-parameter]", which is why I added some switches for those options as well.The following options with the given default values have been introduced:
LIBTRIXI_BUILD_FORTRAN_API=ON;LIBTRIXI_WITH_T8CODE=OFF;LIBTRIXI_ENABLE_WARNINGS=OFF;LIBTRIXI_ENABLE_WERROR=OFF;LIBTRIXI_ENABLE_TESTING=OFFMaybe the changes are interesting for libtrixi, but I am not sure, if I covered everything and if that is the intended way for libtrixi.