Fix define flags in CMakeLists.txt#33
Conversation
Flags added with `add_compile_definitions` shouldn't have a `-D`, since that gets added automatically, but flags included in variables defined with `set` should, since they are used as is.
| set(CMAKE_CXX_FLAGS "${MXLIB_CXXVERSION} ${MXLIB_OPTIMIZE} ${MXLIB_CXXFLAGS}") | ||
|
|
||
| set(MXLIB_PC_CFLAGS "${MXLIB_PC_CFLAGS} ${CMAKE_CXX_FLAGS} ${MXLIB_DEFINES}") | ||
| set(MXLIB_PC_CFLAGS "${MXLIB_PC_CFLAGS} ${CMAKE_CXX_FLAGS} -${MXLIB_DEFINES}") |
There was a problem hiding this comment.
what happens here if MXLIB_DEFINES has more than one thing in it?
There was a problem hiding this comment.
If MXLIB_DEFINES has multiple elements, this would break. Handling multiple elements would require iterating and prepending -D to each. I can define another variable to do that if MXLIB_DEFINES is likely to grow.
|
|
||
| #Don't let Eigen use openmp | ||
| add_compile_definitions(-DEIGEN_DONT_PARALLELIZE) | ||
| add_compile_definitions(DEIGEN_DONT_PARALLELIZE) |
| set(MXLIB_NVCCXX_FLAGS "${MXLIB_NVCCXX_FLAGS} -I${EIGEN3_INCLUDE_DIRS} ${EIGEN3_CFLAGS} ${EIGEN3_CFLAGS_OTHER}") | ||
|
|
||
| set(MXLIB_PC_CFLAGS "${MXLIB_PC_CFLAGS} -I${EIGEN3_INCLUDE_DIRS} ${EIGEN3_CFLAGS} ${EIGEN3_CFLAGS_OTHER} -DEIGEN_DONT_PARALLELIZE") | ||
| set(MXLIB_PC_CFLAGS "${MXLIB_PC_CFLAGS} -I${EIGEN3_INCLUDE_DIRS} ${EIGEN3_CFLAGS} ${EIGEN3_CFLAGS_OTHER} -DDEIGEN_DONT_PARALLELIZE") |
| set(MXLIB_OPTIMIZE "-O3 --fast-math" CACHE STRING "mxlib optimization settings") | ||
|
|
||
| set(MXLIB_DEFINES "-D_XOPEN_SOURCE=700" CACHE STRING "mxlib compiler command line defines") | ||
| set(MXLIB_DEFINES "D_XOPEN_SOURCE=700" CACHE STRING "mxlib compiler command line defines") |
There was a problem hiding this comment.
Actually we don't need the -D. It's manually added on line 365 for the flag definition and on line 360, add_compile_definitions adds its own automatically.
|
I've made the fixes necessary based on your comments. The complication here is due to the variables being used in two different contexts: as input to |
|
yeah maintaining a fully specified pkgconfig file is a pain. I already implemented a bunch of such processors for other flags so I think we can add something that iterates and adds -D if we really need to. According to ChatGPT requiring 3.26 will only be a problem on Ubuntu 22 and then only if we can't update the toolchain. So that is a fine workaround. |
|
thanks for looking at this, btw |
|
Of course! I tested the install on one of our Orins with the 3.26 requirement and it worked the same as for 3.24, so updating the requirement shouldn’t add any extra overhead for us if we end up stuck on U22. (As an aside, discussions are in progress with NSV to update us, but there isn’t a clear path yet). So then we can close this PR unmerged and I’m happy to open another one to update the requirement. How does that sound to you? |
2 participants
Flags added with
add_compile_definitionsshouldn't have a-D, since that gets added automatically, but flags included in variables defined withsetshould, since they are used as is.