Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Build sdformattest pybind11 library as MODULE #1544

Merged
merged 3 commits into from
Feb 14, 2025

Conversation

traversaro
Copy link
Contributor

@traversaro traversaro commented Feb 12, 2025

🦟 Bug fix

Fixes segmentation fault of pyParserConfig_TEST on macOS when building the library against conda-forge dependencies.

Summary

I was investigating a segmentation fault in conda-forge/libsdformat-feedstock#134, and I isolated the problem to the sdformattest test Python module to be defined as a SHARED library. By changing it to a MODULE, the module loads fine (without segfaults) and the test passes fine.

Why the problem only occurs on conda-forge and not with Homebrew's Python (that I guess is used by tests)? I am not completely sure about this, but I guess it boils down to conda-forge using a python executable that statically links libpython, while Homebrew uses a Python that is dynamically linked to libpython, see the related issues:

Beside platform-specific problems, it seems that all other invocations of pybind11_add_module in gz projects use MODULE, so I think it make sense to switch to MODULE also here at least for consistency.

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.

Signed-off-by: Silvio Traversaro <silvio@traversaro.it>
@github-actions github-actions bot added 🏛️ ionic Gazebo Ionic 🪵 jetty Gazebo Jetty labels Feb 12, 2025
@traversaro
Copy link
Contributor Author

I am not sure, but as this is a CMake-only change, I would guess that the Bazel failure is unrelated and actually due to some non-determinism in the Bazel build CI, also considering it also happens in #1543 .

Copy link

codecov bot commented Feb 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (sdf15@2b583cb). Learn more about missing BASE report.

Additional details and impacted files
@@           Coverage Diff            @@
##             sdf15    #1544   +/-   ##
========================================
  Coverage         ?   92.48%           
========================================
  Files            ?      138           
  Lines            ?    18345           
  Branches         ?        0           
========================================
  Hits             ?    16967           
  Misses           ?     1378           
  Partials         ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@scpeters
Copy link
Member

I am not sure, but as this is a CMake-only change, I would guess that the Bazel failure is unrelated and actually due to some non-determinism in the Bazel build CI, also considering it also happens in #1543 .

I think it has started failing since gazebosim/gz-math#652 was merged

@scpeters scpeters enabled auto-merge (squash) February 14, 2025 23:11
@scpeters scpeters merged commit 71fe504 into gazebosim:sdf15 Feb 14, 2025
15 of 16 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
🏛️ ionic Gazebo Ionic 🪵 jetty Gazebo Jetty
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants