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.

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

export: avoid error on unbound variable (IDFGH-9586) #10935

Merged

Conversation

MarcFinetRtone
Copy link
Contributor

Might stop sourcing export.sh when set -u is used (typically with direnv):

/path/to/esp-idf/export.sh:16: IDF_EXPORT_QUIET: unbound variable

@CLAassistant
Copy link

CLAassistant commented Mar 7, 2023

CLA assistant check
All committers have signed the CLA.

@espressif-bot espressif-bot added the Status: Opened Issue is new label Mar 7, 2023
@github-actions github-actions bot changed the title export: avoid error on unbound variable export: avoid error on unbound variable (IDFGH-9586) Mar 7, 2023
@mfialaf
Copy link
Collaborator

mfialaf commented Mar 8, 2023

Hi @MarcFinetRtone ,
Can you please explain in more detail what issue is solving your proposal change?
What is the expected output, and what is the actual without your change?
Thanks

@MarcFinetRtone
Copy link
Contributor Author

MarcFinetRtone commented Mar 8, 2023

Hi @MarcFinetRtone , Can you please explain in more detail what issue is solving your proposal change? What is the expected output, and what is the actual without your change? Thanks

To describe here in the PR or in the commit message ?

Regarding the change:

  • without the change, if set -u is used in the shell, sourcing export.sh stops on the unbound variable (and environment variables are not exported, hence idf.py is not usable). The error message is the following:

/path/to/esp-idf/export.sh:16: IDF_EXPORT_QUIET: unbound variable

  • with the change sourcing export.sh works and idf is usable (because all other environment variables are set)

It's the same problem fixed by 1673183 (but don't know why IDF_QUIET (at the time) was not considered by the commit).

A workaround is to export IDF_EXPORT_QUIET (even to an empty value) prior to sourcing export.sh

@mfialaf
Copy link
Collaborator

mfialaf commented Mar 8, 2023

@MarcFinetRtone Thank you for clearing it up for me, I can see the issue now.

While testing it, I discovered the same problem with the unbound variable with IDF_PATH on line 75.
Can you please also fix this variable so we can merge it as one fix?
Thank you.

I the shell has `set -u` (to abort on unbound variable), sourcing
export.sh currently fails when IDF_EXPORT_QUIET or IDF_PATH is not set:
> /path/to/esp-idf/export.sh:16: IDF_EXPORT_QUIET: unbound variable

This commit sets a default empty value to those variable, as done
in 1673183 (which forgot IDF_PATH as it's usually set, the other
variable landed later in the file).
Copy link
Collaborator

@mfialaf mfialaf left a comment

Choose a reason for hiding this comment

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

@MarcFinetRtone Thank you for your contribution and cooperation.

@dobairoland
Copy link
Collaborator

sha=13e59ed7a99f6bf7ee3d851faeee1fd4b3261ea5

@dobairoland dobairoland added the PR-Sync-Merge Pull request sync as merge commit label Mar 15, 2023
@espressif-bot espressif-bot added Status: Reviewing Issue is being reviewed Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally Resolution: Done Issue is done internally and removed Status: Opened Issue is new Status: Reviewing Issue is being reviewed Resolution: NA Issue resolution is unavailable labels Mar 17, 2023
@espressif-bot espressif-bot merged commit a686db8 into espressif:master Mar 22, 2023
@MarcFinetRtone MarcFinetRtone deleted the fix-export-unbound-variable branch April 13, 2023 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-Sync-Merge Pull request sync as merge commit Resolution: Done Issue is done internally Status: Done Issue is done internally
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants