-
Notifications
You must be signed in to change notification settings - Fork 1.5k
tools/espressif: add esptool version check to Espressif build system #14267
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.
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
tools/espressif: add esptool version check to Espressif build system #14267
Conversation
|
[Experimental Bot, please feedback here] Fill In The Commit Message: This PR contains a Commit with an Empty Commit Message. Please fill in the Commit Message with the PR Summary. The PR summary and impact sections appear to meet NuttX requirements. However, the testing section is insufficient. Here's what needs improvement:
In short, provide concrete evidence that your change works as intended. |
89223ec to
9127152
Compare
|
12.7 release backport #14268 |
|
@fdcavalcanti this PR should target master. The back-port targets 12.7 Please rebase |
9127152 to
cfba4e3
Compare
|
@jerpelea rebased. Please verify. |
|
Thank you @fdcavalcanti !! Works fine :-) Two minor python script tweaks to consider:
Testing on FreeBSD with esptool 4.7.0: With my suggestion on expected version added: With esptool 4.8.1 from venv works fine: |
cederom
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @fdcavalcanti :-) Please update python script, examples provided :-)
|
@fdcavalcanti please update so that we can merge it and unblock the release (I will pick it to 12.7) |
cfba4e3 to
0208695
Compare
|
@jerpelea updated with expected version. Thanks. |
|
@fdcavalcanti something gone really wrong |
0208695 to
92f8f0c
Compare
|
@lupyuen |
92f8f0c to
a959359
Compare
Seems the Python linters are not installed in the CI test virtual environment. The rest looks good. |
@fdcavalcanti So do we need to install them in the Dockerfile right now? https://github.com/apache/nuttx/blob/master/tools/ci/docker/linux/Dockerfile Update: Oops this is the Python Virtual Environment, not the Docker Container |
|
@fdcavalcanti Do we need this to fix Update: Hmmm nope, I already see it here: https://github.com/fdcavalcanti/nuttx/blob/feature/esptool_script_check_min_version/tools/checkpatch.sh#L86 |
It just needs to be installed in a venv, similar to this |
|
@fdcavalcanti The venv is already the latest version I think? I see this in the log below. Where is it picking up the https://github.com/apache/nuttx/actions/runs/11344800359/job/31550311398?pr=14267 |
Its only pip installing cmake (right after enabling the venv), need to also install black, isort, flake8. |
|
@fdcavalcanti Not sure if I can help much? I might make things worse (since I know nothing about Python) @acassis @cederom Do you know who can help with this Python Environment issue? (Simbit is on holiday I think) |
|
@fdcavalcanti please rebase on master |
a959359 to
e01212a
Compare
We're good. |
|
Sorry I was out today on a funeral in a different city. Btw we have esptool 4.8.1 in FreeBSD system packages now: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=282000. Thank you for the fix!! |
Summary
Add a Python script to check if esptool is installed and if its version is up-to-date (see minimum version at ESPTOOL_MIN_VERSION on .mk files).
This fixes build issues as reported in #13824.
Impact
New Python script added. Stops build if esptool version is below 4.8.
Script runs for ESP32, S2, S3, C3, C6.
Testing
Tested locally and on internal CI with Python3.8+.