feat: Adopt cmake for project build system generation by dobairoland · Pull Request #5 · espressif/esp-flasher-stub

dobairoland · 2025-03-18T13:26:16Z

Only three chips are added in this PR in order to keep it clear and easy to review.
There will be some renaming needed after change: add initial source/build structure esp-stub-lib#3. That can be done as part of Build with esp-stub-lib (ESPTOOL-1041) #8.
The build action will fail for the missing JSON files. Follow-up task: Create a JSON file from ELF and add it to the release (ESPTOOL-1042) #9

github-actions · 2025-03-18T13:26:49Z

	Messages
📖	This PR seems to be quite large (total lines of code: 9364), you might consider splitting it into smaller PRs

👋 Hello dobairoland, we appreciate your contribution to this project!

Click to see more instructions ...

This automated output is generated by the PR linter DangerJS, which checks if your Pull Request meets the project's requirements and helps you fix potential issues.

DangerJS is triggered with each push event to a Pull Request and modify the contents of this comment.

Please consider the following:
- Danger mainly focuses on the PR structure and formatting and can't understand the meaning behind your code or changes.
- Danger is not a substitute for human code reviews; it's still important to request a code review from your colleagues.
- Addressing info messages (📖) is strongly recommended; they're less critical but valuable.
- To manually retry these Danger checks, please navigate to the Actions tab and re-run last Danger workflow.

Review and merge process you can expect ...

We do welcome contributions in the form of bug reports, feature requests and pull requests.

1. An internal issue has been created for the PR, we assign it to the relevant engineer.
2. They review the PR and either approve it or ask you for changes or clarifications.
3. Once the GitHub PR is approved we do the final review, collect approvals from core owners and make sure all the automated tests are passing.
- At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
4. If the change is approved and passes the tests it is merged into the default branch.

Generated by 🚫 dangerJS against a812780

Copilot

Pull Request Overview

This PR introduces a new "How to Build" section in the README.md to guide users in building the project with CMake and Ninja.

Added instructions to initialize the esp-stub-lib submodule.
Provided CMake and Ninja command-line steps for generating and building the project.

Files not reviewed (3)

CMakeLists.txt: Language not supported
src/ld/esp32.ld: Language not supported
src/main.cpp: Language not supported

Copilot

Copilot reviewed 9 out of 15 changed files in this pull request and generated 1 comment.

Files not reviewed (6)

CMakeLists.txt: Language not supported
src/CMakeLists.txt: Language not supported
src/ld/esp32.ld: Language not supported
tools/build_all_chips.sh: Language not supported
tools/export_toolchains.sh: Language not supported
tools/setup_toolchains.sh: Language not supported

README.md

Copilot

Copilot reviewed 9 out of 15 changed files in this pull request and generated 1 comment.

Files not reviewed (6)

CMakeLists.txt: Language not supported
src/CMakeLists.txt: Language not supported
src/ld/esp32.ld: Language not supported
tools/build_all_chips.sh: Language not supported
tools/export_toolchains.sh: Language not supported
tools/setup_toolchains.sh: Language not supported

.github/workflows/build_and_release.yml

Copilot

Copilot reviewed 8 out of 16 changed files in this pull request and generated no comments.

Files not reviewed (8)

CMakeLists.txt: Language not supported
src/CMakeLists.txt: Language not supported
src/ld/esp32-c3.ld: Language not supported
src/ld/esp32.ld: Language not supported
src/ld/esp8266.ld: Language not supported
tools/build_all_chips.sh: Language not supported
tools/export_toolchains.sh: Language not supported
tools/setup_toolchains.sh: Language not supported

Comments suppressed due to low confidence (1)

.github/workflows/build_and_release.yml:86

Ensure the artifact path 'artifact/esp*.json' matches the output from the build step, which uses 'build*/esp*.json'. Align these paths to avoid missing files during release.

          files: artifact/esp*.json

dobairoland · 2025-04-08T17:43:36Z

@fhrbata @erhankur PTAL if you have time for this. I'll be happy for any suggestions.

erhankur

@dobairoland left some comments.

erhankur · 2025-04-08T18:18:40Z

CMakeLists.txt

+
+project(flasher-stub CXX)
+
+option(TARGET_CHIP "Target ESP chip" "OFF")


If there are no change requests in the review of the initial structure, esp-stub-lib will require the variable ESP_TARGET for the same purpose.

If I understood your comment correctly then probably there will be a clash. I think we can address this as part of the follow-up task (#8).

We will adjust this project based on espressif/esp-stub-lib#3. No need to do it the other way.

CMakeLists.txt

erhankur · 2025-04-08T18:47:35Z

src/CMakeLists.txt

+    get_target_property(comp_opts esp8266 COMPILE_OPTIONS)
+    list(REMOVE_ITEM comp_opts -Wconversion)
+
+    add_library(miniz_obj OBJECT miniz.cpp)


I think you can remove miniz files.
OpenOCD will also need zlib library. You will be able to call it from the library.

Thank you for pointing this out. I added a comment here for the future.

I'd like to keep this for now. It took me a while to figure out how to change the flags for source files individually. I'd like to keep it here as example. We will try to keep miniz for now, test the new stub and then possible switch if it will be reasonably. But it is also possible that we will keep using miniz until the approaching end-of-life of ESP8266 as the stub for it rarely needs update these days.

src/ld/esp32-c3.ld

erhankur · 2025-04-08T18:57:58Z

tools/setup_toolchains.sh

+#!/usr/bin/env bash
+
+TO_DOWNLOAD=(
+    "https://github.com/espressif/crosstool-NG/releases/download/esp-14.2.0_20241119/xtensa-esp-elf-14.2.0_20241119-x86_64-linux-gnu.tar.xz"


Why don't we suggest to use esp-idf environment for newer chips? Most probably all tools will be already installed.

I'm leaving this open for others to see and comment.

My reasons were:

It was good enough and had low maintenance so far. We just replaced the link time-to-time and were not forced to update every time ESP-IDF did adopt a new toolchain. This helped to track size and see the changes related to only the change of our sources. Automated change of toolchain will break our pipelines comparing the flasher binaries included inside esptool (new warnings, new flags, etc.).

It doesn't suggest to use anything from ESP-IDF.

The environment is clean and simple.

Sounds reasonable. Thanks for the explanation.

dobairoland · 2025-04-09T05:13:07Z

@erhankur Thank you for your useful comments. I tried to address all of them.

fhrbata · 2025-04-09T06:44:34Z

Hi @dobairoland , in general LGTM as something we can start with. I'm not sure if we need to keep the separate CMakeLists.txt in the src directory. The project isn't that big and having it all at one place might be easier to read. What I see as a drawback from the gnu make version is that we cannot build all the stubs at once with this approach. Thank you

dobairoland · 2025-04-09T07:08:53Z

@fhrbata Thank you for taking a look.

I'm not sure if we need to keep the separate CMakeLists.txt in the src directory.

We can remove it later. At this point I'm not sure how complex will be the content of src. We would like to avoid ifdef-ing the sources for every chip target. So that CMakeLists.txt could handle chip-based assembling the sources and headers. We'd also like to have modular flash chip support.

What I see as a drawback from the gnu make version is that we cannot build all the stubs at once with this approach.

I agree on that. In the earlier versions I attempted that and even made cmake download the toolchains. But it was a mess and I simplified it to the current state.

erhankur

@dobairoland thanks for addressing and answering my comments. LGTM

dobairoland force-pushed the feat/simple_cmake_build branch from 3c19780 to b1c25d4 Compare March 18, 2025 15:06

dobairoland linked an issue Mar 19, 2025 that may be closed by this pull request

Adopt cmake for project build system generation (ESPTOOL-1028) #6

Closed

3 tasks

dobairoland force-pushed the feat/simple_cmake_build branch 8 times, most recently from 69afeb3 to a8248eb Compare March 24, 2025 06:10

dobairoland requested a review from Copilot March 24, 2025 07:50

Copilot AI reviewed Mar 24, 2025

View reviewed changes

dobairoland force-pushed the feat/simple_cmake_build branch 2 times, most recently from 5fc41f5 to ba4bd1e Compare March 25, 2025 15:01

dobairoland force-pushed the feat/simple_cmake_build branch 11 times, most recently from e1df9eb to 5488504 Compare April 4, 2025 13:31

dobairoland requested a review from Copilot April 4, 2025 13:39

Copilot AI reviewed Apr 4, 2025

View reviewed changes

README.md Outdated Show resolved Hide resolved

dobairoland force-pushed the feat/simple_cmake_build branch 2 times, most recently from 2ea5c1d to 92f6a46 Compare April 4, 2025 14:01

dobairoland force-pushed the feat/simple_cmake_build branch 2 times, most recently from 21feabf to 3d1871c Compare April 4, 2025 14:14

dobairoland requested a review from Copilot April 4, 2025 14:21

Copilot AI reviewed Apr 4, 2025

View reviewed changes

.github/workflows/build_and_release.yml Show resolved Hide resolved

dobairoland force-pushed the feat/simple_cmake_build branch 3 times, most recently from 0109c8e to 1b40bd5 Compare April 8, 2025 14:02

dobairoland mentioned this pull request Apr 8, 2025

Create a JSON file from ELF and add it to the release (ESPTOOL-1042) #9

Closed

3 tasks

dobairoland force-pushed the feat/simple_cmake_build branch from 1b40bd5 to 22f01ca Compare April 8, 2025 17:25

dobairoland requested a review from Copilot April 8, 2025 17:25

Copilot AI reviewed Apr 8, 2025

View reviewed changes

dobairoland marked this pull request as ready for review April 8, 2025 17:34

dobairoland force-pushed the feat/simple_cmake_build branch from 22f01ca to d9cc956 Compare April 8, 2025 17:40

dobairoland requested a review from radimkarnis April 8, 2025 17:41

erhankur reviewed Apr 8, 2025

View reviewed changes

dobairoland force-pushed the feat/simple_cmake_build branch from d9cc956 to 07a2940 Compare April 9, 2025 04:44

feat: Adopt cmake for project build system generation

a812780

dobairoland force-pushed the feat/simple_cmake_build branch from 07a2940 to a812780 Compare April 9, 2025 07:24

erhankur approved these changes Apr 9, 2025

View reviewed changes

dobairoland merged commit 58247c1 into master Apr 23, 2025
4 of 6 checks passed

dobairoland deleted the feat/simple_cmake_build branch April 23, 2025 11:13

Copilot AI mentioned this pull request Feb 10, 2026

Update copilot instructions to reflect current repository state (ESPTOOL-1270) #43

Merged

30 tasks


		project(flasher-stub CXX)

		option(TARGET_CHIP "Target ESP chip" "OFF")

Conversation

dobairoland commented Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

dobairoland commented Apr 8, 2025

Uh oh!

erhankur left a comment

Choose a reason for hiding this comment

Uh oh!

erhankur Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

dobairoland Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erhankur Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

dobairoland Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

erhankur Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

dobairoland Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

erhankur Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

dobairoland commented Apr 9, 2025

Uh oh!

fhrbata commented Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dobairoland commented Apr 9, 2025

Uh oh!

erhankur left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dobairoland commented Mar 18, 2025 •

edited

Loading

github-actions bot commented Mar 18, 2025 •

edited

Loading

dobairoland Apr 9, 2025 •

edited

Loading

fhrbata commented Apr 9, 2025 •

edited

Loading