Skip to content

zephyr: CMake portability fixes - #6611

Closed
rleigh-lumiradx wants to merge 1 commit into
micropython:esp32-idf41-cmakefrom
rleigh-lumiradx:cmake-sed
Closed

rleigh-lumiradx wants to merge 1 commit into
micropython:esp32-idf41-cmakefrom
rleigh-lumiradx:cmake-sed

Conversation

@rleigh-lumiradx

Copy link
Copy Markdown

@dpgeorge This PR is based upon the comments I made in #6542 (comment)

It's not directly mergeable, since there were differences between the two branches I've not yet fully reconciled. It looks like some changes were merged which obsoletes some of what I was doing. However, I hope some of the general ideas here are useful:

  • The main part is the last bit which adds quote and dequote scripts to replace the use of sed.
  • shell commands like echo and sed are replaced with CMake-native replacements. This provides for portability beyond UNIX e.g. to Windows
  • some configuration currently done at build-time can be done at configure-time, e.g. version header generation. There's no real need for this to be a build rule. This means it won't regenerate the header upon every invocation of the build. I added a bit of logic to make it automatically regenerate whenever the git HEAD changes, which means it will still happen, but less frequently. That could be extended to also include other sources of version information, e.g. documentation.
  • looking more closely, I do wonder if all of the qstr processing couldn't be done at configure time and regenerated on demand if needed

@rleigh-lumiradx

Copy link
Copy Markdown
Author

One additional comment. Some variables used by mkrules.cmake are defined in ports/esp32/CMakeLists.txt and are not usable by other ports, e.g. zephyr. I wonder if some of these couldn't be factored out into a more general location.

One thought I had: Why is the CMake support port-specific? Why not have a generic CMake build which can be used for any platform, and then as the basis for customisation in individual ports. mkrules.cmake gets us part of the way there, but it could be a bit more general than this.

@dpgeorge

Copy link
Copy Markdown
Member

@rleigh-lumiradx now that cmake support is in master, can you please rebase this on master and then I can do a review?

@rleigh-lumiradx

Copy link
Copy Markdown
Author

I'll have a look, and see if this is now necessary--it may already have been addressed by the CMake support branch.

@dpgeorge dpgeorge closed this May 18, 2021
@dpgeorge
dpgeorge deleted the branch micropython:esp32-idf41-cmake May 18, 2021 01:41
tannewt added a commit to tannewt/circuitpython that referenced this pull request Aug 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants