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

[windows][master] Use .pyd as the output suffix for Python module on Windows. #1637

Merged
merged 2 commits into from
Aug 27, 2019

Conversation

seanyen
Copy link
Contributor

@seanyen seanyen commented Aug 21, 2019

The .pyd is the file extension for Python extension modules on Windows. This change is to fix the file extension on Windows.

@seanyen seanyen requested a review from rhaschke as a code owner August 21, 2019 07:39
Copy link
Contributor

@v4hn v4hn left a comment

Choose a reason for hiding this comment

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

The problem of handling the separate suffixes looks like cmake should provide an abstraction for it already, given that this is required for all python modules on windows everywhere.

However, it looks like the patch you provide is still the standard way to handle this.

@v4hn v4hn added the awaits 2nd review one maintainer approved this request label Aug 21, 2019
Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

Generally, I approve this PR.
What about introducing a cmake macro that marks a library target as a python lib, which allows to hide the complexity / boringness of these three lines behind a single line:
mark_target_as_python_extension(TARGET)

The macro should be defined by ros_core and thus would be automatically included by all packages that depend on ros_core.
On the other hand, this hiding might be not desired. And we might have name clashes if other packages will define such a macro as well.

@v4hn
Copy link
Contributor

v4hn commented Aug 22, 2019

The macro should be defined by ros_core and thus would be automatically included by all packages that depend on ros_core.

Sounds like a good idea, I'm not sure you will find someone to review such an addition for ros_comm for ROS1, but at least for ROS2 this seems like a great idea.

I would propose to merge this request nonetheless until such a feature becomes available somewhere.

@rhaschke
Copy link
Contributor

Sorry, I meant moveit_core instead of ros_core. I think, for us, it will be sufficient to have such a macro in MoveIt. I agree with you, @v4hn, that probably nobody will agree to augment ros_comm or even catkin with such a cmake macro.

@v4hn
Copy link
Contributor

v4hn commented Aug 27, 2019

Merging with two approvals.

@rhaschke I do not really like the idea of adding this macro only within MoveIt.
Custom project macros always make it much harder to understand the build structure of big code bases.
Until now, MoveIt did a great job at sticking to "standard" cmake/catkin.

@v4hn v4hn merged commit e2bfd71 into moveit:master Aug 27, 2019
henningkayser pushed a commit to PickNikRobotics/moveit that referenced this pull request Nov 21, 2019
…#1637)

There is no other standard way to mark this in cmake
rhaschke pushed a commit to ubi-agni/moveit that referenced this pull request Feb 8, 2020
…#1637)

There is no other standard way to mark this in cmake
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaits 2nd review one maintainer approved this request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants