dist/tools: improve the esptools, enhance documentation#21625
Merged
crasbe merged 1 commit intoRIOT-OS:masterfrom Jul 28, 2025
Merged
dist/tools: improve the esptools, enhance documentation#21625crasbe merged 1 commit intoRIOT-OS:masterfrom
esptools, enhance documentation#21625crasbe merged 1 commit intoRIOT-OS:masterfrom
Conversation
crasbe
added
Type: enhancement
github-actions
bot
added
Area: doc
gschorcht
reviewed
Jul 26, 2025
gschorcht
requested changes
Jul 26, 2025
Contributor
|
I still wonder whether the output regarding the permanent setting of the PATH variable should be inside of the @benpicco added these lines there, maybe he has an strong opinion. My suggestion would be that could be part of this PR: diff --git a/dist/tools/esptools/export.sh b/dist/tools/esptools/export.sh
index defd527abf..dc04187327 100755
--- a/dist/tools/esptools/export.sh
+++ b/dist/tools/esptools/export.sh
@@ -47,10 +47,10 @@ export_arch()
if [ -e "${TOOLS_DIR}" ] && [ -z "${TOOLS_DIR_IN_PATH}" ]; then
echo "Extending PATH by ${TOOLS_DIR}/bin"
export PATH="${TOOLS_DIR}/bin:${PATH}"
- fi
- echo "To make this permanent, add this line to your ~/.bashrc or ~/.profile:"
- echo PATH="\$PATH:${TOOLS_DIR}/bin"
+ echo "To make this permanent, add this line to your ~/.bashrc or ~/.profile:"
+ echo PATH="\$PATH:${TOOLS_DIR}/bin"
+ fi
unset TOOLS_DIR
} |
Contributor
Author
|
Sure, why not. I'll add it later. |
Contributor
Author
|
I also added the version check now, but this has to be tested more extensively. |
crasbe
added
the
State: WIP
Contributor
Author
|
@gschorcht one thing I noticed is that no warning etc is printed when the toolchain is not present. For example, I don't have Perhaps that should at least print an error message? |
Contributor
Yes, but this is only true for the additional tools, that is, We have just to these lines also to |
Contributor
|
BTW, since the export_tool()
{
if [ ! -e "${TOOLS_DIR}" ]; then
echo "${TOOLS_DIR} does not exist - please run"
echo "\${RIOTBASE}/dist/tools/esptools/install.sh $1"
exit(1)
fi
echo "$PATH" | tr ':' '\n' | while read -r entry; do
if echo "$entry" | grep -q "^${TOOLS_PATH}/${TARGET_ARCH}/.*/${TARGET_ARCH}/bin$"; then
if [ "$entry" != "${TOOLS_DIR}/bin" ]; then
echo "Warning: PATH contains outdated entry: \"$entry\"." \
"Please check your ~/.bashrc or ~/.profile.">&2
fi
fi
done
unset entry
if [ -e "${TOOLS_DIR}" ] && [ -z "${TOOLS_DIR_IN_PATH}" ]; then
echo "Extending PATH by ${TOOLS_DIR}/bin"
export PATH="${TOOLS_DIR}/bin:${PATH}"
echo "To make this permanent, add this line to your ~/.bashrc or ~/.profile:"
echo PATH="\$PATH:${TOOLS_DIR}/bin"
fi
}It makes no sense to repeat the same code again and again. |
Contributor
Author
|
The new Note here: There was a bug in |
crasbe
removed
the
State: WIP
crasbe
changed the title
esptools, enhance documentation
Contributor
Not really a bug, it stems from a time when no additional parameters were required for the architecture because |
gschorcht
requested changes
Jul 28, 2025
Contributor
Author
|
Should be correct now: I also added the distinction between |
gschorcht
approved these changes
Jul 28, 2025
Contributor
gschorcht
left a comment
gschorcht
left a comment
There was a problem hiding this comment.
Looks good, please squash.
The esptools/export.sh script has to be sourced, otherwise the exported variables are not persistent in the environment, rendering the script useless. The commit adds a check to the script that it runs in the correct environment and enhances the documentation to make it clearer that the "source" prefix command is important. The PR also introduces a version check for the PATH variable to notify the user if an old version is present.
crasbe
force-pushed
the
pr/esptools_export
branch
from
6524a6f to
bd0f8b7
Compare
Contributor
|
Thanks a lot for your work on these scripts. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Contribution description
The esptools/export.sh script has to be sourced, otherwise the exported variables are not persistent in the environment, rendering the script useless. The commit adds a check to the script that it runs in the correct environment and enhances the documentation to make it clearer that the "source" prefix command is important.
The documentation mentioned that, but I did not understand what "sourcing a script" means and I did not pay attention to the dot in front of the script. Sorry @gschorcht for not reading the documentation well enough 👀
Open for discussion: Should the script terminate before doing anything (as it does now) or should it print out the warning instead of the
Extending PATH by ...message?Testing procedure: Sourcing
Pre: Make sure your PATH variable does not contain the latest ESP SDK version yet.
Current situation on
master: Calling thedist/tools/esptools/export.shscript without a.orsourcecommand in the front suggests to be adding the SDK path to the PATH variable, while it actually doesn't (well it does, but to the environment of the script, which is lost after the script terminates):With this PR, an error message is printed:
Testing Procedure: Version Check
Testing procedure:
Issues/PRs references
Came up in #21616 (comment)