-
Notifications
You must be signed in to change notification settings - Fork 113
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
Stop using python_cmake_module. #93
base: rolling
Are you sure you want to change the base?
Conversation
@clalancette thank you for doing this! It looks like this PR removes the usage of |
That's a really good question. Most of the other places I did this already had separate calls to |
Ah, I see. So it turns out that deep in the bowels of But it doesn't seem right to depend on that; theoretically, someday Relatedly, I realize that this package is missing a dependency on |
So we discussed this. First, it is our feeling that downstream packages that just depend on The other question had to do about whether we should explicitly do So I think this PR is generally ready to go. It still needs CI run on it, and we'll almost certainly have to merge in ros2/ci#755 first, but I think it is otherwise OK. @jonbinney let me know what you think. |
@clalancette that sounds good to me and this seems mergeable, but I realized this isn't actually one of the packages I maintain so @mabelzhang should make the call on merging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, will run CI after the ros2/ci PR gets merged
We really don't need it anymore, and can just use the builtin find_package(Python3). Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
5377535
to
3969274
Compare
3969274
to
cb69be1
Compare
cb69be1
to
3969274
Compare
We really don't need it anymore, and can just use the builtin find_package(Python3).
This must be merged before ros2/ros2#1524 ; see that pull request for more information about this change.