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 various Python wheel generation issues #1188

Merged
merged 10 commits into from
Sep 24, 2024

Conversation

kylc
Copy link
Contributor

@kylc kylc commented Sep 23, 2024

@zkingston I also pulled in your changes to fix an error in the Python binding generation due to the recent changes in include paths. These affected the CI wheel generation too, but I can wait and rebase if you want to merge those separately.

Follow here to see a successful CI build: https://github.com/kylc/ompl/actions/runs/10998657273

.github/workflows/wheels.yaml Outdated Show resolved Hide resolved
@zkingston
Copy link
Member

Thanks @kylc! Happy to have those changes I made put here. Something we might also want to consider is dropping the pinned version of pygccxml, and forcing only clang to be used as the compiler for the bindings (since castxml is not playing nice with modern gcc). See:

Copy link
Member

@zkingston zkingston left a comment

Choose a reason for hiding this comment

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

LGTM. Not sure what is going on with the failed Mac build - cannot replicate that on my local machine.

@@ -49,6 +49,7 @@ def build_extension(self, ext: CMakeExtension) -> None:
# EXAMPLE_VERSION_INFO shows you how to pass a value into the C++ code
# from Python.
cmake_args = [
"-DCMAKE_CXX_COMPILER=/usr/bin/clang++", # Force Clang for castxml
Copy link
Member

Choose a reason for hiding this comment

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

Probably want to have this be a variable with the found clang executable, not explicitly hardcoding this path.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe move the below clang search to above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that we build the macos/arm64 target natively instead of cross-compiling, it seem like we can use the system clang installation instead of the one from homebrew. The clang binary search logic is now gone in c817f36.

I've replaced this bit with a shutil.where("clang++") in 8740aa8.

@kylc
Copy link
Contributor Author

kylc commented Sep 23, 2024

Pulled in your changes mostly as-is. I also bumped the CastXML version in case that separately resolves the GCC issues; I see some GCC-related commits recently in the CastXML repo.

Will remove the WIP commit after the wheel job succeeds on my fork and then we can get this merged. Follow: https://github.com/kylc/ompl/actions/runs/11002295331

If you can take a look at ompl/pyplusplus#2, it fixes some errors with the Python bindings on MacOS that have been reported as issues here.

@zkingston
Copy link
Member

Mark got there before me, but looks good to go!

@kylc
Copy link
Contributor Author

kylc commented Sep 23, 2024

Thanks, ready to merge!

@zkingston zkingston merged commit 18a5f5b into ompl:main Sep 24, 2024
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants