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

Improve error messages when embedSdf.py fails #1549

Merged
merged 3 commits into from
Mar 4, 2025

Conversation

scpeters
Copy link
Member

@scpeters scpeters commented Mar 3, 2025

🦟 Bug fix

Follow-up to #1536.

Summary

The embedSdf.py script is used to generate the EmbeddedSdf.cc source code that implements the interface defined in src/EmbeddedSdf.hh. The contribution by @ efferre79 in #1536 adds error checking to confirm that the generated EmbeddedSdf.cc file is not empty. This pull request makes some additional improvements to this error checking.

a44ccb2: check that EmbeddedSdf.cc exists before checking its size, use string comparisons, and rephrase the error message

Currently the file(SIZE) call generates a cmake error if the EmbeddedSdf.cc file does not exist and the if(${OUT_SIZE} EQUAL 0) comparison fails if OUT_SIZE is an empty variable:

CMake Error at sdf/CMakeLists.txt:28 (file):
  file SIZE requested of path that is not readable:

    <CMAKE_BINARY_DIR>/src/EmbeddedSdf.cc


CMake Error at sdf/CMakeLists.txt:29 (if):
  if given arguments:

    "EQUAL" "0"

  Unknown arguments specified

After a44ccb2, the error message when EmbeddedSdf.cc is empty or does not exist changes to:

CMake Error at sdf/CMakeLists.txt:34 (message):
  <CMAKE_BINARY_DIR>/src/EmbeddedSdf.cc is
  empty or does not exist

1ca9bb6: check return code of embedSdf.py and print stderr

This checks the RESULT_VARIABLE of the execute_process call and prints the ERROR_VARIABLE if it is not successful. After this change, the error message is:

CMake Error at sdf/CMakeLists.txt:33 (message):
  Error executing
  /usr/local/Frameworks/Python.framework/Versions/3.13/bin/python3.13
  /Users/scpeters/ws/sdformat_math/src/sdformat/sdf/embedSdf.py to create
  /Users/scpeters/ws/sdformat_math/src/sdformat/build/src/EmbeddedSdf.cc:
  Traceback (most recent call last):

    File "/Users/scpeters/ws/sdformat_math/src/sdformat/sdf/embedSdf.py", line 13, in <module>
      intentional_error

  NameError: name 'intentional_error' is not defined

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@scpeters scpeters requested a review from azeey as a code owner March 3, 2025 23:12
@github-actions github-actions bot added 🏛️ ionic Gazebo Ionic 🪵 jetty Gazebo Jetty labels Mar 3, 2025
Signed-off-by: Steve Peters <[email protected]>
@efferre79
Copy link
Contributor

Looks good to me!

@scpeters scpeters merged commit bf054f0 into sdf15 Mar 4, 2025
17 checks passed
@scpeters scpeters deleted the scpeters/embed_py_error_check branch March 4, 2025 17:33
@scpeters
Copy link
Member Author

scpeters commented Mar 4, 2025

we can backport 1ca9bb6, since that should work with older cmake versions

scpeters added a commit that referenced this pull request Mar 4, 2025
Check embedSdf.py return code and print stderr on failure.
Partial backport of #1549.

Signed-off-by: Steve Peters <[email protected]>
@scpeters
Copy link
Member Author

scpeters commented Mar 4, 2025

we can backport 1ca9bb6, since that should work with older cmake versions

partial backport in #1550

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏛️ ionic Gazebo Ionic 🪵 jetty Gazebo Jetty
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants