Change meson target configs #81
Conversation
viega
changed the title
There was a problem hiding this comment.
I understood the PR description as "CI now runs with ASan and UBSan", and that sounds good to me. But that's not currently happening, right?
I'd also expect some errors in the logs if the sanitizers were running. For example, from #56, when UBSan is enabled I think we'd want to add to the c_args:
-fno-sanitize=function
|
Something weird has happened to the Mac runner; it no longer finds libffi. @ee7 lmk if you see what might be the issue on that. |
|
The below diff to 7db4669 makes the macOS build work again. Does that help? -function meson_init_target {
+function meson_build {
+
+ echo ${1} > .meson_last
+ rm deps/local 2>/dev/null
+ ln -s ${PWD}/deps/${OS}-${ARCH} ${PWD}/deps/local
+
+
if [[ ! -d ${1} ]]; then
if [[ -f ${1} ]]; then
rm -rf ${1}
@@ -41,16 +47,7 @@ function meson_init_target {
log meson setup ${@}
meson setup ${@}
fi
-}
-function meson_build {
-
- echo ${1} > .meson_last
- rm deps/local 2>/dev/null
- ln -s ${PWD}/deps/${OS}-${ARCH} ${PWD}/deps/local
-
-
- meson_init_target ${@}
cd ${1}
log Compiling meson target ${1}
@@ -187,9 +184,7 @@ case $1 in
meson_build debug
debug_it
;;
- testbuild) meson_init_target cicd --buildtype=debug
- meson configure cicd -Duse_memcheck=true
- meson_build cicd
+ testbuild) meson_build cicd --buildtype=debug
;; |
1. The debug target does NOT turn on ASAN / UBSAN by default (the noise was slowing down debugging a lot). This *does* have our own internal memory checking enabled though. 2. There's a new 'cicd' target. It's accessible via `dev testbuild` 3. There's a separate target for enabling ASAN/UBSAN. That's not guaranteed to give good results. CI/CD config is set up to use #2.
|
Was helpful @ee7, thanks. Passing now. |
There was a problem hiding this comment.
LGTM, with a couple of non-blocking questions from before.
Nit for merging: For the merged commit title, can we avoid using the current PR title as-is? I would understand
Remove ASAN / UBSan from debug; add it to CI/CD
as "CI now uses ASan and UBSan", but that won't be the case.
| c_args = c_args + ['-fsanitize=undefined', | ||
| '-fsanitize-recover=all' | ||
| ] | ||
| '-fsanitize-recover=all' | ||
| ] | ||
| link_args = link_args + ['-fsanitize=undefined', | ||
| '-fsanitize-recover=all' | ||
| ] |
There was a problem hiding this comment.
Nit: Indentation-wise, I was referring to the start of line 78, not the horizontal alignment of lines 79 and 80. But my understanding is the next minor meson release (1.5.0) will include a meson format command anyway.
| @@ -162,7 +158,7 @@ function libcon4m_run_tests { | |||
| } | |||
|
|
|||
| function libcon4m_dev_clean { | |||
| for item in build debug release; do | |||
| for item in build debug release cicd .meson_last; do | |||
| if [[ -d ${item} ]]; then | |||
There was a problem hiding this comment.
Nit: .meson_last in the line above still looks strange to me.
Doesn't that always do nothing, because .meson.last is a file? Is the intention to read the contents of .meson.last, like TARGET elsewhere in this file?
viega
changed the title
Changed the meson build targets so that:
dev testbuildCI/CD config is set up to use the second of these.