zephyr/CMakeLists.txt: remove '..' in include paths #7428

marc-hb · 2023-04-10T23:36:33Z

zephyr/CMakeLists.txt: remove '..' in include paths

CMake seems to behave differently on Linux and Windows: it generates
different -I command line parameters. This results in spurious
__FILE__ mismatches and non-reproducible builds when using
CONFIG_ASSERT, see example in #7428.

On Windows, '..' seem resolved more often which also seems to convert
forward slashes to backslashes.

They are also less readable and wasting a bit of space. Remove them
using cmake_path(SET ...)

Zephyr's -fmacro-prefix-map is working and keeps the builds reproducible when using a recent enough toolchain.

As found in the CONFIG_ASSERT PR #6530 (comment) this is not true for old Xtensa toolchains which don't support -fmacro-prefix-map

marc-hb · 2023-04-11T00:08:07Z

Some zephyr.lst differences in https://github.com/thesofproject/sof/actions/runs/4662244951/jobs/8252512487?pr=7428, interesting...

marc-hb · 2023-04-11T00:58:33Z

The ASSERTs built on Windows have a mix of forward and backward slashes. There are 63 WEST_TOPDIR paths with forward slashes and 6 with backward slashes:

strings build-sof-staging/sof-info/mtl/stripped-zephyr.elf | grep TOPDIR

...
WEST_TOPDIR/sof/src/library_manager/lib_notification.c
WEST_TOPDIR/sof/src/samples/audio/detect_test.c
WEST_TOPDIR/sof/src/schedule/zephyr_ll.c
WEST_TOPDIR/sof/zephyr/include/rtos/task.h
WEST_TOPDIR/sof/zephyr/lib/cpu.c
WEST_TOPDIR/sof/zephyr/wrapper.c
WEST_TOPDIR/zephyr/arch/common/sw_isr_common.c
WEST_TOPDIR/zephyr/arch/xtensa/core/xtensa-asm2.c
WEST_TOPDIR/zephyr/arch/xtensa/include/kernel_arch_func.h
WEST_TOPDIR/zephyr/drivers/dai/intel/dmic/dmic.c

...
WEST_TOPDIR/zephyr/subsys/logging/log_output.c
WEST_TOPDIR/zephyr/subsys/pm/device_runtime.c
WEST_TOPDIR/zephyr/subsys/pm/pm.c
WEST_TOPDIR/zephyr/subsys/pm/policy.c
WEST_TOPDIR\sof\src\include\sof\audio\audio_stream.h
WEST_TOPDIR\sof\src\include\sof\audio\component_ext.h
WEST_TOPDIR\sof\src\include\sof\coherent.h
WEST_TOPDIR\sof\src\include\sof\schedule\ll_schedule_domain.h
WEST_TOPDIR\sof\src\platform\intel\ace\include\ace\lib\mailbox.h
WEST_TOPDIR\sof\xtos\include\sof\lib\mailbox.h

@stephanosio any idea?

EDIT, found something: all the paths with backward slashes have at least one .. on Linux:

WEST_TOPDIR/sof/src/lib/notifier.c
WEST_TOPDIR/sof/src/library_manager/lib_notification.c
WEST_TOPDIR/sof/src/samples/audio/detect_test.c
WEST_TOPDIR/sof/src/schedule/zephyr_ll.c
WEST_TOPDIR/sof/zephyr/../src/../xtos/include/sof/lib/mailbox.h
WEST_TOPDIR/sof/zephyr/../src/include/sof/audio/audio_stream.h
WEST_TOPDIR/sof/zephyr/../src/include/sof/audio/component_ext.h
WEST_TOPDIR/sof/zephyr/../src/include/sof/coherent.h
WEST_TOPDIR/sof/zephyr/../src/include/sof/schedule/ll_schedule_domain.h
WEST_TOPDIR/sof/zephyr/../src/platform/intel/ace/include/ace/lib/mailbox.h
WEST_TOPDIR/sof/zephyr/include/rtos/task.h
WEST_TOPDIR/sof/zephyr/lib/cpu.c
WEST_TOPDIR/sof/zephyr/wrapper.c

EDIT: difference fixed by removing .. in sof/zephyr/CMakeLists.txt.

CMake seems to behave differently on Linux and Windows: it generates different `-I` command line parameters. This results in spurious `__FILE__` mismatches and non-reproducible builds when using CONFIG_ASSERT, see example in thesofproject#7428. On Windows, '..' seem resolved more often which also seems to convert forward slashes to backslashes. They are also less readable and wasting a bit of space. Remove them using cmake_path(SET ...) Signed-off-by: Marc Herbert <marc.herbert@intel.com>

kv2019i

Looks good. One question , see inline comment

kv2019i · 2023-04-11T09:51:17Z

.github/workflows/zephyr.yml

@@ -131,7 +131,7 @@ jobs:
      - name: build
        run: cd workspace && ./sof/zephyr/docker-run.sh
             ./sof/zephyr/docker-build.sh --cmake-args=-DEXTRA_CFLAGS=-Werror
-             --cmake-args=--warn-uninitialized ${{ matrix.IPC_platforms }}
+             --cmake-args=--warn-uninitialized -d ${{ matrix.IPC_platforms }}


@marc-hb Should we have both -d and non -d builds? If we can only have one, I'd say the non-debug build is more important to verify.

It should be easy to build both -d and not but I'm concerned about the combinatorial explosion and making the small "checks" box at the bottom of the "Conversation" tab more unreadable than it already is. Or we could connsider that box a lost cause and get people into the habit of alway switching to the "Checks" tab? I could also save some space but grouping all IPC3 (imx and tgl) together.

Note quickbuild already builds without -d every time, however its log is often hard to find. This being said, I don't remember a case when -d was OK while the build failed without or vice-versa, so convenient access to logs of a problem that almost never happens is not critical.

Thoughts?

I agree with @kv2019i on this one. As you said Marc, building whole array with -d flag would result in so many builds...
I think we should stick with just a "production" build.
EDIT:
And now I think about all those ASSERT macros that would not be verified by the "production" build.
@marc-hb I think we should run both. I know it's a lot of builds but well, its common practice for projects to build production and debug builds. Besides, I think debug_overlay may grow in future, we need to verify this too.

Github Actions allow "sparse" matrices. So I found a way to add -d only for MTL, submitted in

[SKIP CI] .github/zephyr: build with the debug overlay and CONFIG_ASSERT #7508

Make sure users do not misinterpret our single `debug_overlay.conf` file as some kind of complex and elaborate "debug build" concept. Also save users who want to see the file a lot of time by naming it directly in the --help. Empty layers of indirection are just spurious obfuscation. Signed-off-by: Marc Herbert <marc.herbert@intel.com>

marc-hb · 2023-04-14T18:20:31Z

@kv2019i , @aborisovich I removed the github checks changes for now, let's discuss them later in another PR. It's more urgent to fix CONFIG_ASSERT reproducibility than to test it. So this PR should be a no-brainer now.

EDIT, see also:

[SKIP CI] .github/zephyr: move zephyr revision in first results matrix position #7446

let's discuss them later in another PR.
EDIT:

[SKIP CI] .github/zephyr: build with the debug overlay and CONFIG_ASSERT #7508

CMake seems to behave differently on Linux and Windows: it generates different `-I` command line parameters. This results in spurious `__FILE__` mismatches and non-reproducible builds when using CONFIG_ASSERT, see example in #7428. On Windows, '..' seem resolved more often which also seems to convert forward slashes to backslashes. They are also less readable and wasting a bit of space. Remove them using cmake_path(SET ...) Signed-off-by: Marc Herbert <marc.herbert@intel.com>

marc-hb requested review from aborisovich and kv2019i April 11, 2023 01:03

marc-hb mentioned this pull request Apr 11, 2023

Verify builds are reproducible in the CI zephyrproject-rtos/zephyr#50205

Open

marc-hb force-pushed the gha-assert branch from 102945e to 806ee89 Compare April 11, 2023 07:28

marc-hb marked this pull request as ready for review April 11, 2023 07:29

marc-hb requested review from dbaluta, cujomalainey and lgirdwood as code owners April 11, 2023 07:29

marc-hb changed the title ~~[SKIP CI] .github/zephyr: build with the debug overlay and CONFIG_ASSERT~~ .github/zephyr: build with the debug overlay and CONFIG_ASSERT Apr 11, 2023

marc-hb requested review from andyross, lyakh, keqiaozhang, dcpleung and aiChaoSONG April 11, 2023 07:31

marc-hb changed the title ~~.github/zephyr: build with the debug overlay and CONFIG_ASSERT~~ zephyr/CMakeLists.txt: remove '..' in include paths Apr 11, 2023

kv2019i reviewed Apr 11, 2023

View reviewed changes

aborisovich approved these changes Apr 12, 2023

View reviewed changes

marc-hb force-pushed the gha-assert branch from 806ee89 to 894a948 Compare April 14, 2023 18:17

kv2019i approved these changes Apr 17, 2023

View reviewed changes

kv2019i merged commit c3c7b33 into thesofproject:main Apr 24, 2023
31 of 35 checks passed

marc-hb deleted the gha-assert branch April 24, 2023 15:30

This was referenced Dec 8, 2023

meta issue for reproducible builds (was: tests/determinism.sh) zephyrproject-rtos/zephyr#14593

Closed

audio: crossover: crossover code refactor #8587

Closed

This was referenced Apr 11, 2024

nxp: imx8/imx8x: switch to Zephyr logging #9021

Closed

Windows/Linux disassembly differences when using #include ../something.h? #9034

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zephyr/CMakeLists.txt: remove '..' in include paths #7428

zephyr/CMakeLists.txt: remove '..' in include paths #7428

marc-hb commented Apr 10, 2023 •

edited

marc-hb commented Apr 11, 2023

marc-hb commented Apr 11, 2023 •

edited

kv2019i left a comment

kv2019i Apr 11, 2023

marc-hb Apr 11, 2023 •

edited

aborisovich Apr 12, 2023 •

edited

marc-hb Apr 24, 2023

marc-hb commented Apr 14, 2023 •

edited

zephyr/CMakeLists.txt: remove '..' in include paths #7428

zephyr/CMakeLists.txt: remove '..' in include paths #7428

Conversation

marc-hb commented Apr 10, 2023 • edited

marc-hb commented Apr 11, 2023

marc-hb commented Apr 11, 2023 • edited

kv2019i left a comment

Choose a reason for hiding this comment

kv2019i Apr 11, 2023

Choose a reason for hiding this comment

marc-hb Apr 11, 2023 • edited

Choose a reason for hiding this comment

aborisovich Apr 12, 2023 • edited

Choose a reason for hiding this comment

marc-hb Apr 24, 2023

Choose a reason for hiding this comment

marc-hb commented Apr 14, 2023 • edited

marc-hb commented Apr 10, 2023 •

edited

marc-hb commented Apr 11, 2023 •

edited

marc-hb Apr 11, 2023 •

edited

aborisovich Apr 12, 2023 •

edited

marc-hb commented Apr 14, 2023 •

edited