Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ESP32: Deprecate make support #7034

Merged
merged 1 commit into from
Jun 2, 2021
Merged

ESP32: Deprecate make support #7034

merged 1 commit into from
Jun 2, 2021

Conversation

sweetymhaiske
Copy link
Contributor

Problem

  • Currently ESP32 supports both CMake and Make build systems.
  • Developer need to update build files for both build systems, which is in a way tedious.
  • ESP-IDF is highly CMake compliant, hence to enhance performance by focusing on a single build system Make support is being deprecated.
  • Fixes ESP32: Remove the support of make for examples #6850

Change overview

  1. Removed below files from all esp32 examples
    • Makefile
    • component.mk
    • idf.sh
  2. Made needed changes for successful CI.

Testing

  • Compiled all esp32 examples using CMake (idf.py)
  • Successful CI

@restyled-io restyled-io bot mentioned this pull request May 21, 2021
Closed
@sweetymhaiske sweetymhaiske force-pushed the deprecate_make branch 5 times, most recently from 98ab556 to 9dc74e6 Compare May 21, 2021 10:10
@restyled-io restyled-io bot mentioned this pull request May 21, 2021
Closed
@sweetymhaiske sweetymhaiske force-pushed the deprecate_make branch 9 times, most recently from 632e674 to 3d60ee3 Compare May 21, 2021 13:27
@sweetymhaiske
Copy link
Contributor Author

sweetymhaiske commented May 21, 2021 •
edited
Loading

Github CI fails with this error https://github.com/project-chip/connectedhomeip/runs/2639767629?check_suite_focus=true#step:4:504 How can it be resolved?

@andy31415
Copy link
Contributor

Github CI fails with this error https://github.com/project-chip/connectedhomeip/runs/2639767629?check_suite_focus=true#step:4:504 How can it be resolved?

I believe we need to update the docker image. Discussed with @dhrishi - I believe updating the esp32 images should work (and tie them to a commit instead of a branch).

dhrishi
dhrishi requested changes May 21, 2021
View reviewed changes
src/test_driver/esp32/qemu_setup.sh Outdated Show resolved Hide resolved
src/test_driver/esp32/qemu_setup.sh Outdated Show resolved Hide resolved
scripts/examples/esp_example.sh Outdated Show resolved Hide resolved
@arunbharadwaj
Copy link
Contributor

Is the README at https://github.com/project-chip/connectedhomeip/tree/master/examples/all-clusters-app/esp32 still accurate regarding the instructions on how to build esp32 image after this change? If not, can you please update it as well? Thanks.

@dhrishi
Copy link
Contributor

dhrishi commented May 21, 2021

Is the README at https://github.com/project-chip/connectedhomeip/tree/master/examples/all-clusters-app/esp32 still accurate regarding the instructions on how to build esp32 image after this change? If not, can you please update it as well? Thanks.

@arunbharadwaj We had previously removed the mention of "idf make" from the README.md so that the new folks use CMake. But we can check if things need to be cleaned up there. Thanks! cc @sweetymhaiske

@sweetymhaiske sweetymhaiske marked this pull request as ready for review May 21, 2021 16:28
@sweetymhaiske sweetymhaiske force-pushed the deprecate_make branch 2 times, most recently from b6bdb75 to 529ec11 Compare May 27, 2021 12:06
@sweetymhaiske
Copy link
Contributor Author

@sweetymhaiske Why is src/test_driver/esp32/idf.sh in the changes, if you have not done any changes there?

@dhrishi resolved. Thanks

@dhrishi
Copy link
Contributor

dhrishi commented May 27, 2021

LGTM. @andreilitvin Can you please review this? @rgoliver This PR is ready now. So you should be unblocked.

@sweetymhaiske sweetymhaiske requested a review from dhrishi May 27, 2021 13:14
andy31415
andy31415 approved these changes May 27, 2021
View reviewed changes
scripts/examples/esp_example.sh Outdated
@@ -31,16 +31,21 @@ fi

source "scripts/activate.sh"
# shellcheck source=/dev/null
source "$root"/idf.sh
cd "$IDF_PATH"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does source "${IDF_PATH}/export.sh" work to save some cd operations?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should work, IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

scripts/examples/esp_example.sh Outdated
for sdkconfig in "$root"/sdkconfig*.defaults; do
# remove root path to get sdkconfig*.defaults name
sdkconfig_name=${sdkconfig#"$root"/}
rm -f "$root"/sdkconfig
SDKCONFIG_DEFAULTS=$sdkconfig_name idf make -j8 -C "$root" defconfig "$@"
idf make -j8 -C "$root" "$@" || {
cd "$root"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it may be nice to avoid changing directories in case someone marks a script with "-e" (which is frequently used).

Could you run these in a subshell? Like:

(cd "$root"; idf.py -D SDKCONFIG_DEFAULTS="$sdkconfig_name" build)
(cd "$root"; idf.py build "$@") || { 
    # ...

I am not sure if the latter works, however if it does then we do not need the extra "cd" operations

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andy31415 it works. I have made the needed changes.

@dhrishi dhrishi mentioned this pull request May 27, 2021
Closed
@restyled-io restyled-io bot mentioned this pull request May 28, 2021
Closed
@andy31415
Copy link
Contributor

@sweetymhaiske - merge conflicts - should be able to merge once those are fixed.

@sweetymhaiske sweetymhaiske force-pushed the deprecate_make branch 2 times, most recently from df60213 to cae76d7 Compare June 1, 2021 13:21
@sweetymhaiske
Copy link
Contributor Author

@sweetymhaiske - merge conflicts - should be able to merge once those are fixed.

@andy31415 Resolved.

@dhrishi
Copy link
Contributor

dhrishi commented Jun 2, 2021

@sweetymhaiske There seems to be a merge conflict again.

@woody-apple woody-apple self-requested a review June 2, 2021 05:05
@sweetymhaiske
Copy link
Contributor Author

@sweetymhaiske There seems to be a merge conflict again.

Resolved.

@andy31415 andy31415 merged commit 5d54fac into project-chip:master Jun 2, 2021
@sweetymhaiske sweetymhaiske mentioned this pull request Jun 28, 2021
Merged
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
@sweetymhaiske
ESP32: Deprecate make support (project-chip#7034)
b432800
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhrishi dhrishi dhrishi approved these changes

@woody-apple woody-apple woody-apple approved these changes

@msandstedt msandstedt msandstedt approved these changes

@andy31415 andy31415 andy31415 approved these changes

@bzbarsky-apple bzbarsky-apple Awaiting requested review from bzbarsky-apple

@chrisdecenzo chrisdecenzo Awaiting requested review from chrisdecenzo

@Damian-Nordic Damian-Nordic Awaiting requested review from Damian-Nordic

@hawk248 hawk248 Awaiting requested review from hawk248

@jepenven-silabs jepenven-silabs Awaiting requested review from jepenven-silabs

Assignees
No one assigned
Labels
config examples integrations review - approved scripts
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ESP32: Remove the support of make for examples
6 participants
@sweetymhaiske @andy31415 @arunbharadwaj @dhrishi @msandstedt @woody-apple