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

Fix: esptool_py incorrectly assumed target name equals binary name (IDFGH-11420) #12558

Conversation

itavero
Copy link
Contributor

@itavero itavero commented Nov 10, 2023

While integrating the ESP-IDF into our existing CMake structure, I've come across quite some hurdles.
Most I've been able to fix in our CMake files, however this one I could not.

Most of the targets created by the esptool_py component assume that the EXECUTABLE IDF build property (which contains the name of the CMake executable target) always equals the name of the created binary.

This is however not always true. For instance, in our setup we use CMAKE_EXECUTABLE_SUFFIX_C and CMAKE_EXECUTABLE_SUFFIX_CXX in our toolchain file (both set to .elf).
If we do add_executable(my_app), the target binary file would actually be my_app.elf.

In order to fix this, I've updated it to use the TARGET_FILE generated expression. That way we also no longer need the EXECUTABLE_DIR IDF build property here.

I've fixed this on v5.0.1 (as that's the ESP-IDF version I'm currently trying to integrate), but I assume it should be easy to apply the same fix to newer versions and the master branch as well.

Note that this problem might exist in multiple places where EXECUTABLE is being used.
While going through the ESP-IDF code base, I even noticed that a few places actually already seem to use the TARGET_FILE expression.
To be honest the property name might be somewhat confusing as well, as it is actually the executable target.

@CLAassistant
Copy link

CLAassistant commented Nov 10, 2023

CLA assistant check
All committers have signed the CLA.

@espressif-bot espressif-bot added the Status: Opened Issue is new label Nov 10, 2023
@github-actions github-actions bot changed the title Fix: esptool_py incorrectly assumed target name equals binary name Fix: esptool_py incorrectly assumed target name equals binary name (IDFGH-11420) Nov 10, 2023
@igrr
Copy link
Member

igrr commented Nov 10, 2023

Thanks for the fix @itavero!

We can't merge the PR as is, because we have to first make the fix in the master branch. We will rebase your commit to master and later will backport the change to the existing release branches (release/v5.2, release/v5.1, release/v5.0).

@itavero itavero force-pushed the fix-esptoolpy-incorrect-assumption-about-target-name branch from fbdcdc2 to 5293eff Compare November 10, 2023 12:57
@itavero itavero changed the base branch from release/v5.0 to master November 10, 2023 12:57
Copy link

Messages
📖 Good Job! All checks are passing!

👋 Welcome itavero, thank you for your first contribution to espressif/esp-idf project!

📘 Please check Contributions Guide for the contribution checklist, information regarding code and documentation style, testing and other topics.

🖊️ Please also make sure you have read and signed the Contributor License Agreement for espressif/esp-idf project.

Pull request review and merge process you can expect

Espressif develops the ESP-IDF project in an internal repository (Gitlab). We do welcome contributions in the form of bug reports, feature requests and pull requests via this public GitHub repository.

  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 synchronize it into our internal git repository
  4. In the internal git repository 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.
  5. If the change is approved and passes the tests it is merged into the master branch
  6. On next sync from the internal git repository merged change will appear in this public Github repository

Generated by 🚫 dangerJS against 5293eff

@itavero
Copy link
Contributor Author

itavero commented Nov 10, 2023

@igrr I've rebased it on master and did a force push. Looking forward to a backport to v5.0.

@espressif-bot espressif-bot added Status: Selected for Development Issue is selected for development and removed Status: Opened Issue is new labels Nov 13, 2023
@fhrbata
Copy link
Collaborator

fhrbata commented Nov 15, 2023

sha=5293eff4cdd8545266bafdec6fe98522bbaf9c28

@fhrbata fhrbata added the PR-Sync-Merge Pull request sync as merge commit label Nov 15, 2023
@espressif-bot espressif-bot added Status: In Progress Work is in progress Status: Reviewing Issue is being reviewed and removed Status: Selected for Development Issue is selected for development Status: In Progress Work is in progress labels Nov 15, 2023
@espressif-bot espressif-bot added Status: Done Issue is done internally Resolution: NA Issue resolution is unavailable and removed Status: Reviewing Issue is being reviewed labels Nov 22, 2023
espressif-bot pushed a commit that referenced this pull request Dec 1, 2023
From: Arno Moonen <arno.moonen@airios.eu>

Follows original message from Arno Moonen <arno.moonen@airios.eu>

While integrating the ESP-IDF into our existing CMake structure,
I've come across quite some hurdles. Most I've been able to fix
in our CMake files, however this one I could not.

Most of the targets created by the esptool_py component assume
that the EXECUTABLE IDF build property (which contains the name
of the CMake executable target) always equals the name of the
created binary.

This is however not always true. For instance, in our setup we use
CMAKE_EXECUTABLE_SUFFIX_C and CMAKE_EXECUTABLE_SUFFIX_CXX in our
toolchain file (both set to .elf). If we do add_executable(my_app),
the target binary file would actually be my_app.elf.

In order to fix this, I've updated it to use the TARGET_FILE generated
expression. That way we also no longer need the EXECUTABLE_DIR IDF build
property here.

I've fixed this on v5.0.1 (as that's the ESP-IDF version I'm currently
trying to integrate), but I assume it should be easy to apply the same
fix to newer versions and the master branch as well.

Note that this problem might exist in multiple places where EXECUTABLE
is being used. While going through the ESP-IDF code base, I even noticed
that a few places actually already seem to use the TARGET_FILE expression.
To be honest the property name might be somewhat confusing as well, as it
is actually the executable target.

Closes #12558
movsb pushed a commit to movsb/esp-idf that referenced this pull request Dec 1, 2023
From: Arno Moonen <arno.moonen@airios.eu>

Follows original message from Arno Moonen <arno.moonen@airios.eu>

While integrating the ESP-IDF into our existing CMake structure,
I've come across quite some hurdles. Most I've been able to fix
in our CMake files, however this one I could not.

Most of the targets created by the esptool_py component assume
that the EXECUTABLE IDF build property (which contains the name
of the CMake executable target) always equals the name of the
created binary.

This is however not always true. For instance, in our setup we use
CMAKE_EXECUTABLE_SUFFIX_C and CMAKE_EXECUTABLE_SUFFIX_CXX in our
toolchain file (both set to .elf). If we do add_executable(my_app),
the target binary file would actually be my_app.elf.

In order to fix this, I've updated it to use the TARGET_FILE generated
expression. That way we also no longer need the EXECUTABLE_DIR IDF build
property here.

I've fixed this on v5.0.1 (as that's the ESP-IDF version I'm currently
trying to integrate), but I assume it should be easy to apply the same
fix to newer versions and the master branch as well.

Note that this problem might exist in multiple places where EXECUTABLE
is being used. While going through the ESP-IDF code base, I even noticed
that a few places actually already seem to use the TARGET_FILE expression.
To be honest the property name might be somewhat confusing as well, as it
is actually the executable target.

Closes espressif#12558
igrr pushed a commit that referenced this pull request Dec 6, 2023
From: Arno Moonen <arno.moonen@airios.eu>

Follows original message from Arno Moonen <arno.moonen@airios.eu>

While integrating the ESP-IDF into our existing CMake structure,
I've come across quite some hurdles. Most I've been able to fix
in our CMake files, however this one I could not.

Most of the targets created by the esptool_py component assume
that the EXECUTABLE IDF build property (which contains the name
of the CMake executable target) always equals the name of the
created binary.

This is however not always true. For instance, in our setup we use
CMAKE_EXECUTABLE_SUFFIX_C and CMAKE_EXECUTABLE_SUFFIX_CXX in our
toolchain file (both set to .elf). If we do add_executable(my_app),
the target binary file would actually be my_app.elf.

In order to fix this, I've updated it to use the TARGET_FILE generated
expression. That way we also no longer need the EXECUTABLE_DIR IDF build
property here.

I've fixed this on v5.0.1 (as that's the ESP-IDF version I'm currently
trying to integrate), but I assume it should be easy to apply the same
fix to newer versions and the master branch as well.

Note that this problem might exist in multiple places where EXECUTABLE
is being used. While going through the ESP-IDF code base, I even noticed
that a few places actually already seem to use the TARGET_FILE expression.
To be honest the property name might be somewhat confusing as well, as it
is actually the executable target.

Closes #12558
espressif-bot pushed a commit that referenced this pull request Dec 13, 2023
From: Arno Moonen <arno.moonen@airios.eu>

Follows original message from Arno Moonen <arno.moonen@airios.eu>

While integrating the ESP-IDF into our existing CMake structure,
I've come across quite some hurdles. Most I've been able to fix
in our CMake files, however this one I could not.

Most of the targets created by the esptool_py component assume
that the EXECUTABLE IDF build property (which contains the name
of the CMake executable target) always equals the name of the
created binary.

This is however not always true. For instance, in our setup we use
CMAKE_EXECUTABLE_SUFFIX_C and CMAKE_EXECUTABLE_SUFFIX_CXX in our
toolchain file (both set to .elf). If we do add_executable(my_app),
the target binary file would actually be my_app.elf.

In order to fix this, I've updated it to use the TARGET_FILE generated
expression. That way we also no longer need the EXECUTABLE_DIR IDF build
property here.

I've fixed this on v5.0.1 (as that's the ESP-IDF version I'm currently
trying to integrate), but I assume it should be easy to apply the same
fix to newer versions and the master branch as well.

Note that this problem might exist in multiple places where EXECUTABLE
is being used. While going through the ESP-IDF code base, I even noticed
that a few places actually already seem to use the TARGET_FILE expression.
To be honest the property name might be somewhat confusing as well, as it
is actually the executable target.

Closes #12558
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: NA Issue resolution is unavailable Status: Done Issue is done internally
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants