Thanks for looking into this, it does look quite good, a good step to make.

It is similar to #6392 (using an ExternalProject) but I think the approach here is cleaner because here there is no dependency on make, it's all pure cmake.

Well, except for the qstr processing needing to run the C preprocessor, that is tricky. It seems that cmake doesn't have a portable/generic way of specifying that a file needs preprocessing, and so qstr preprocessing needs to be defined as a custom command that runs cc -E. That's ok, custom commands are not a big issue, but then it means needing to know the relevant C flags/defines/options to also pass to cc -E. The "hack" I did for this on the esp32 only works with make, not eg ninja as found out here.

I think I have a solution which is less of a hack but still not that great. The fix looks like this (which needs to be cleaned up but gives you the idea):

diff --git a/py/mkrules.cmake b/py/mkrules.cmake
index 8822cbeee..72134bef1 100644
--- a/py/mkrules.cmake
+++ b/py/mkrules.cmake
@@ -46,12 +46,23 @@ add_custom_command(
 
 # Generate qstrs
 
+get_property_and_add_prefix(micropython_includes_raw ${MICROPYTHON_TARGET} INCLUDE_DIRECTORIES "-I")
+process_flags(C micropython_includes_raw micropython_includes)
+get_property_and_add_prefix(micropython_definitions_raw ${MICROPYTHON_TARGET} COMPILE_DEFINITIONS "-D")
+process_flags(C micropython_definitions_raw micropython_definitions)
+zephyr_get_include_directories_for_lang(C includes)
+zephyr_get_system_include_directories_for_lang(C system_includes)
+zephyr_get_compile_definitions_for_lang(C definitions)
+zephyr_get_compile_options_for_lang(C options)
+
+set(THE_FLAGS ${micropython_includes} ${micropython_definitions} ${includes} ${system_includes} ${definitions} ${options})
+
 # If any of the dependencies in this rule change then the C-preprocessor step must be run.
 # It only needs to be passed the list of SOURCE_QSTR files that have changed since it was
 # last run, but it looks like it's not possible to specify that with cmake.
 add_custom_command(
     OUTPUT ${MPY_QSTR_DEFS_LAST}
-    COMMAND ${CMAKE_C_COMPILER} -E \$\(C_INCLUDES\) \$\(C_FLAGS\) \$\(C_DEFINES\) -DNO_QSTR ${SOURCE_QSTR} > ${MPY_GENHDR_DIR}/qstr.i.last
+    COMMAND ${CMAKE_C_COMPILER} -E ${THE_FLAGS} -DNO_QSTR ${SOURCE_QSTR} > ${MPY_GENHDR_DIR}/qstr.i.last
     DEPENDS ${MPY_MODULEDEFS}
         ${SOURCE_QSTR}
     VERBATIM
@@ -74,7 +85,7 @@ add_custom_command(
 
 add_custom_command(
     OUTPUT ${MPY_QSTR_DEFS_PREPROCESSED}
-    COMMAND cat ${MPY_PY_QSTRDEFS} ${MPY_QSTR_DEFS_COLLECTED} | sed "s/^Q(.*)/\"&\"/" | ${CMAKE_C_COMPILER} -E \$\(C_INCLUDES\) \$\(C_FLAGS\) \$\(C_DEFINES\) - | sed "s/^\\\"\\(Q(.*)\\)\\\"/\\1/" > ${MPY_QSTR_DEFS_PREPROCESSED}
+    COMMAND cat ${MPY_PY_QSTRDEFS} ${MPY_QSTR_DEFS_COLLECTED} | sed "s/^Q(.*)/\"&\"/" | ${CMAKE_C_COMPILER} -E ${THE_FLAGS} - | sed "s/^\\\"\\(Q(.*)\\)\\\"/\\1/" > ${MPY_QSTR_DEFS_PREPROCESSED}
     DEPENDS ${MPY_QSTR_DEFS_COLLECTED}
     VERBATIM
 )

With that patch I was able to build Zephyr using west/ninja.

As for the frozen code issue, that should be much easier to fix.

I think I have a solution which is less of a hack but still not that great. The fix looks like this (which needs to be cleaned up but gives you the idea):

This will work for zephyr, but how will it work for the esp32 port?

As for the frozen code issue, that should be much easier to fix.

I pushed an update to fix this properly

This will work for zephyr, but how will it work for the esp32 port?

The variable ${THE_FLAGS} (to be renamed to something more descriptive) would need to be provided by the port, with perhaps a sensible default provided by mkrules.cmake. I'll look into that.

I've now improved the core cmake files, they can be found on the esp32-idf41-cmake branch of this repository (as the first commit on that branch). Changes are:

• all cmake variables now start with MICROPY_
• used Python3_EXECUTABLE
• made a (hopefully) generic way for a port to specify the options to pass to the preprocessor, via MICROPY_CPP_FLAGS and/or MICROPY_CPP_FLAGS_EXTRA
• made processing of the frozen manifest optional (thanks to @MaureenHelm 's commit here)

@MaureenHelm would you be able to update this PR based on this new cmake code? To get the qstr preprocessing working I think you can add something like this to the end of CMakeLists.txt:

zephyr_get_include_directories_for_lang(C includes)
zephyr_get_system_include_directories_for_lang(C system_includes)
zephyr_get_compile_definitions_for_lang(C definitions)
zephyr_get_compile_options_for_lang(C options)

set(MICROPY_CPP_FLAGS_EXTRA ${includes} ${system_includes} ${definitions} ${options})

include(${MICROPY_DIR}/py/mkrules.cmake)

If you can't get to it let me know and I'll make a new PR based on this one.

One final portability issue is the use of backslashes with sed. The following change makes it use CMake primitives directly, avoiding the need for lots of escaping. While I put the scripts inline in the CMakeLists.txt to keep everything in one place, they could be easily split out if desired which would make the quoting there less complex as well. With this change, it's possible to build on Windows with ninja (on top of all the other changes).

From 72193cf4bdbcd8e45341402717f0df1111516255 Mon Sep 17 00:00:00 2001
From: Roger Leigh <roger.leigh@lumiradx.com>
Date: Thu, 29 Oct 2020 18:05:17 +0000
Subject: [PATCH] Generate qstrdefs.preprocessed.h using CMake natively

Change-Id: I94bd22e0cf556110a074c0878048684e66b7f8b3
---
 ext/micropython/mkrules.cmake | 44 ++++++++++++++++++++++++++++++++---
 1 file changed, 41 insertions(+), 3 deletions(-)

diff --git a/ext/micropython/mkrules.cmake b/ext/micropython/mkrules.cmake
index d445a92..8b01ea1 100644
--- a/ext/micropython/mkrules.cmake
+++ b/ext/micropython/mkrules.cmake
@@ -3,12 +3,14 @@
 find_package(Python3 REQUIRED COMPONENTS Interpreter)

 set(MPY_PY_QSTRDEFS "${MPY_PY_DIR}/qstrdefs.h")
+set(MPY_QSTRDEFS_CONCAT "${CMAKE_CURRENT_BINARY_DIR}/qstrdefs_concat.h")
 set(MPY_GENHDR_DIR "${CMAKE_BINARY_DIR}/genhdr")
 set(MPY_MPVERSION "${MPY_GENHDR_DIR}/mpversion.h")
 set(MPY_MODULEDEFS "${MPY_GENHDR_DIR}/moduledefs.h")
 set(MPY_QSTR_DEFS_LAST "${MPY_GENHDR_DIR}/qstr.i.last")
 set(MPY_QSTR_DEFS_SPLIT "${MPY_GENHDR_DIR}/qstr.split")
 set(MPY_QSTR_DEFS_COLLECTED "${MPY_GENHDR_DIR}/qstrdefs.collected.h")
+set(MPY_QSTR_DEFS_PREPROCESSED_QUOTED "${MPY_GENHDR_DIR}/qstrdefs.preprocessed_quoted.h")
 set(MPY_QSTR_DEFS_PREPROCESSED "${MPY_GENHDR_DIR}/qstrdefs.preprocessed.h")
 set(MPY_QSTR_DEFS_GENERATED "${MPY_GENHDR_DIR}/qstrdefs.generated.h")
 set(MPY_FROZEN_CONTENT "${CMAKE_BINARY_DIR}/frozen_content.c")
@@ -93,14 +95,50 @@ add_custom_command(
     VERBATIM
 )

+file(GENERATE OUTPUT "${CMAKE_CURRENT_BINARY_DIR}/gen_qstrdefs_concat_quote.cmake"
+    CONTENT "# Generate ${MPY_QSTRDEFS_CONCAT} with Q() quotes
+file(REMOVE \"${MPY_QSTRDEFS_CONCAT}\")
+foreach(file \"${MPY_PY_QSTRDEFS}\" \"${MPY_QSTR_DEFS_COLLECTED}\")
+    file(STRINGS \"\${file}\" CONTENTS)
+    foreach(line \${CONTENTS})
+        # Apply regex to quote Q()s
+        string(REGEX REPLACE \"^(Q\\\\(.*\\\\))\" \"\\\"\\\\1\\\"\" replaced \"\${line}\")
+        file(APPEND \"${MPY_QSTRDEFS_CONCAT}\" \"\${replaced}\\n\")
+    endforeach()
+endforeach()
+")
+
+file(GENERATE OUTPUT "${CMAKE_CURRENT_BINARY_DIR}/gen_qstrdefs_dequote.cmake"
+    CONTENT "# Generate ${MPY_QSTR_DEFS_PREPROCESSED} from ${MPY_QSTR_DEFS_PREPROCESSED_QUOTED} with quotes removed
+file(REMOVE \"${MPY_QSTR_DEFS_PREPROCESSED}\")
+file(STRINGS \"${MPY_QSTR_DEFS_PREPROCESSED_QUOTED}\" CONTENTS)
+foreach(line \${CONTENTS})
+    # Apply regex to dequote Q()s
+    string(REGEX REPLACE \"^\\\"(Q\\\\(.*\\\\))\\\"\" \"\\\\1\" replaced \"\${line}\")
+    file(APPEND \"${MPY_QSTR_DEFS_PREPROCESSED}\" \"\${replaced}\\n\")
+endforeach()
+")
+
+add_custom_command(OUTPUT "${MPY_QSTRDEFS_CONCAT}"
+    COMMAND "${CMAKE_COMMAND}" -P "${CMAKE_CURRENT_BINARY_DIR}/gen_qstrdefs_concat_quote.cmake"
+    DEPENDS "${MPY_PY_QSTRDEFS}" "${MPY_QSTR_DEFS_COLLECTED}" "${CMAKE_CURRENT_BINARY_DIR}/gen_qstrdefs_concat_quote.cmake"
+    VERBATIM
+)
+
 add_custom_command(
-    OUTPUT ${MPY_QSTR_DEFS_PREPROCESSED}
-    COMMAND cat ${MPY_PY_QSTRDEFS} ${MPY_QSTR_DEFS_COLLECTED} | sed "s/^Q(.*)/\"&\"/" | ${CMAKE_C_COMPILER} -E ${THE_FLAGS} - | sed "s/^\\\"\\(Q(.*)\\)\\\"/\\1/" > ${MPY_QSTR_DEFS_PREPROCESSED}
-    DEPENDS ${MPY_QSTR_DEFS_COLLECTED}
+    OUTPUT ${MPY_QSTR_DEFS_PREPROCESSED_QUOTED}
+    COMMAND ${CMAKE_C_COMPILER} -E ${THE_FLAGS} ${MPY_QSTRDEFS_CONCAT} > ${MPY_QSTR_DEFS_PREPROCESSED_QUOTED}
+    DEPENDS "${MPY_QSTRDEFS_CONCAT}"
     VERBATIM
     COMMAND_EXPAND_LISTS
 )

+add_custom_command(OUTPUT "${MPY_QSTR_DEFS_PREPROCESSED}"
+        COMMAND "${CMAKE_COMMAND}" -P "${CMAKE_CURRENT_BINARY_DIR}/gen_qstrdefs_dequote.cmake"
+        DEPENDS "${MPY_QSTR_DEFS_PREPROCESSED_QUOTED}" "${CMAKE_CURRENT_BINARY_DIR}/gen_qstrdefs_dequote.cmake"
+        VERBATIM
+        )
+
 add_custom_command(
     OUTPUT ${MPY_QSTR_DEFS_GENERATED}
     COMMAND ${Python3_EXECUTABLE} ${MPY_PY_DIR}/makeqstrdata.py ${MPY_QSTR_DEFS_PREPROCESSED} > ${MPY_QSTR_DEFS_GENERATED}
--
2.27.0.windows.1

@MaureenHelm would you be able to update this PR based on this new cmake code?

@dpgeorge I've updated it, but I'm having trouble getting the qstr preprocessing to work. Would you mind having a look?

I opened https://github.com/NXPmicro/micropython/pull/1/files as a way to provide some suggestions for making the qstr preprocessing work portably, plus a few other tweaks. I hope it's helpful.

I pushed another portability fix (use "cmake -E touch" as a portable alternative to "touch") to the above PR, and also adjusted the path for one of the intermediate qstr files.

@MaureenHelm would you be able to update this PR based on this new cmake code?

@dpgeorge I've updated it, but I'm having trouble getting the qstr preprocessing to work. Would you mind having a look?

I finally got this to work, but it required a change on the zephyr side. See zephyrproject-rtos/zephyr#29935

I opened NXPmicro/micropython#1 (files) as a way to provide some suggestions for making the qstr preprocessing work portably, plus a few other tweaks. I hope it's helpful.

@rleigh-lumiradx Thank you. I think you should submit the sed changes directly to the upstream micropython repo. They're orthogonal changes to mine so they should apply cleanly, and that way you can take credit for the enhancement in the git history.

I finally got this to work, but it required a change on the zephyr side

Great, thanks! I did spend quite some time trying to get this to work, with no success. My last resort (which is not too bad) was going to be to modify py/makeqstrdefs.py so that it split any string arguments passed to it (eg convert "-D1 -D2" into "-D1" "-D2"), and also ignore empty arguments which can sometimes be passed from the Zephyr variables. We can still do that if necessary.

Rebased to master now that the esp32 cmake changes have been merged.

finally got this to work, but it required a change on the zephyr side

Great, thanks! I did spend quite some time trying to get this to work, with no success. My last resort (which is not too bad) was going to be to modify py/makeqstrdefs.py so that it split any string arguments passed to it (eg convert "-D1 -D2" into "-D1" "-D2"), and also ignore empty arguments which can sometimes be passed from the Zephyr variables. We can still do that if necessary.

Pulled in #6797 to pick up the zephyr fix for this issue (needed to pass CI)

This PR is now ready to go!

Merged in 2aa5793 through f573e73 (with a minor change to the last commit to also remove the now-unneeded .gitignore).

Thank you very much for this, a very nice change IMO!

Thank you!