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 PythonLibs include #45

Closed
wants to merge 1 commit into from
Closed

fix PythonLibs include #45

wants to merge 1 commit into from

Conversation

v4hn
Copy link

@v4hn v4hn commented Jun 20, 2014

_FIND_VERSION seems to be unsupported in cmake 3.0. Also there is no
reason for this variable in this case anyway.

<PACKAGE>_FIND_VERSION seems to be unsupported in
cmake 3.0. Also there is no reason for this variable in this case anyway.
@v4hn
Copy link
Author

v4hn commented Jun 20, 2014

Also please re-release this package with indigo to include the change.
Is there a specific reason why hydro still uses 0.2.22?
If there is not, please release this change in hydro as well.

@@ -1,5 +1,4 @@
set(PythonLibs_FIND_VERSION "${PYTHON_VERSION_MAJOR}.${PYTHON_VERSION_MINOR}")
find_package(PythonLibs REQUIRED)
find_package(PythonLibs "${PYTHON_VERSION_MAJOR}.${PYTHON_VERSION_MINOR}" REQUIRED)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this require an exact version match then?

find_package(PythonLibs "${PYTHON_VERSION_MAJOR}.${PYTHON_VERSION_MINOR}" EXACT REQUIRED)

Copy link
Author

Choose a reason for hiding this comment

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

On Sat, Jun 21, 2014 at 05:28:25AM -0700, Dirk Thomas wrote:

Shouldn't this require an exact version match then?

find_package(PythonLibs "${PYTHON_VERSION_MAJOR}.${PYTHON_VERSION_MINOR}" EXACT REQUIRED)

That doesn't work over here at least.
That is because 2.7 != 2.7.7 and find_package detects
the latter one over here. If there is no extra flag available for that case,
I'd like to leave it that way.

@dirk-thomas
Copy link
Contributor

I have committed a modified fix which also sets Python_ADDITIONAL_VERSIONS before to ensure that versions not available in the CMake config file are found.

@v4hn
Copy link
Author

v4hn commented Jun 30, 2014

Thank you for your response and fix.

Although I don't understand why you did not simply merge the request.
The line you've added (1.) has nothing to do with cmake 3.0 and
(2.) is superfluous as long as you respect your distribution and don't install unsupported versions
in official locations.

@v4hn v4hn deleted the fix-pythonlibs branch June 30, 2014 18:15
@dirk-thomas
Copy link
Contributor

Yes, I could have merged the PR first and then added the additional versions line separately. I simply committed them together since it keeps these kind of related changes in a single commit. It was also slightly faster to do it this way since I already had all the changes in my local working copy in order to test it. But anyway I don't see a real difference in both approaches.

And setting the additional versions is by no means superfluous - it is simply not possible to only install a single Python version on most OS I have seen (e.g. Trusty). In order the specific the specific version which should be used (which might not be known to the PythonLibs CMake config file) it must be specified explicitly.

@v4hn
Copy link
Author

v4hn commented Jul 4, 2014

Ok.

It would be great to see this problem fixed in the upcoming indigo-release as well.
However, this requires another release. I don't know whether you expect any more
changes to this module before indigo, but I would like to request this release either way.

@dirk-thomas
Copy link
Contributor

This will be released to all active ROS distros: Groovy, Hydro and Indigo.

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.

None yet

2 participants