robotpkg

So this is breaking sot-dynamic-pinocchio-v3

OK. I need to understand the exact purpose of those headers in order
to find which package should install them and where. It's sound
awkward that a python package provides C++ headers :)

In any case they should be in a directory not conflicting among
different python interpreters. (e.g. include/pinocchio/py[0-9][0-9] or
just ${PYTHON_SITELIB} itself).

For now, there is a "DEPEND_ABI.python+= python<3" in
py-pinocchio/Makefile, so we don't have to fear for any conflict…

For now yes, but the problem will arise sooner or later. Better handle
it properly from the beginning :)

IMHO this is due to the fact that py-pinocchio depends on py-eigen
which is compiled only for 2.7 python interpreter. 

I don't get that: py-eigenpy is compiled for all available python
interpreters, and there are py[0-9][0-9]-eigenpy packages.

While we are working on making py-eigen and py-pinocchio compiling
for several python interpreters, would it be possible to have the
same policy for py-pinocchio than the one used for py-eigen ?

Yes, that's the plan. For py-eigenpy the restriction on python<3 was
artificial and created only by the cmake file. It's fixed locally in
robotpkg with patch-a{c,d}

This would amount to forbid other binary packages than 2.7 for
py-pinocchio. The attached patch implements this.

This patch does not look good: it forbids multiple python versions to
cohabit, which is the opposite of what should be achieved :)

On 02/05/2018 11:31, Anthony Mallet wrote:

Issue #158 has been updated by Anthony Mallet.

So this is breaking sot-dynamic-pinocchio-v3

OK. I need to understand the exact purpose of those headers in order
to find which package should install them and where. It's sound
awkward that a python package provides C++ headers :)

This is not a pure python package.
It provides bindings between a C++ library (pinocchio)and python.
Thus it should be pinocchio to provide the headers.
It used to be the case in wip.

In any case they should be in a directory not conflicting among
different python interpreters. (e.g. include/pinocchio/py[0-9][0-9] or
just ${PYTHON_SITELIB} itself).

Agreed wrt to py-pinocchio

For now, there is a "DEPEND_ABI.python+= python<3" in
py-pinocchio/Makefile, so we don't have to fear for any conflict…

For now yes, but the problem will arise sooner or later. Better handle
it properly from the beginning :)

IMHO this is due to the fact that py-pinocchio depends on py-eigen
which is compiled only for 2.7 python interpreter. 

I don't get that: py-eigenpy is compiled for all available python
interpreters, and there are py[0-9][0-9]-eigenpy packages.

Sorry my misunderstanding.

While we are working on making py-eigen and py-pinocchio compiling
for several python interpreters, would it be possible to have the
same policy for py-pinocchio than the one used for py-eigen ?

Yes, that's the plan. For py-eigenpy the restriction on python<3 was
artificial and created only by the cmake file. It's fixed locally in
robotpkg with patch-a{c,d}

This would amount to forbid other binary packages than 2.7 for
py-pinocchio. The attached patch implements this.

This patch does not look good: it forbids multiple python versions to
cohabit, which is the opposite of what should be achieved :)

I think it is indeed bad, and the proper way is to fix the PLIST of
pinocchio.
If you want I can revert it and patch the pinocchio's PLIST if you agree.

----------------------------------------
Bug #158: Re: Updating HPP in Robotpkg
https://git.openrobots.org/issues/158#change-270

• Author: Guilhem Saurel
• Status: New
• Priority: Normal
• Assignee:
• Category:
• Target version:
  ----------------------------------------
  Mail de Anthony Mallet du jeudi 2018-04-26 19:14:

« at the moment, the headers provided by the python bindings are not
installed because they would conflict between multiple python version. The .pc
file is not installed either for the same reason. If any of those files are
actually needed, one should figure out a proper namespace or a .pc
name that do not conflict. »

/local/users/gsaurel/robotpkg/robotpkg/wip/sot-dynamic-pinocchio-v3/work.akasan/sot-dynamic-pinocchio-v3-3.1.7/src/python-module-py.cpp:21:57: fatal error: pinocchio/bindings/python/multibody/model.hpp: No such file or directory

So this is breaking sot-dynamic-pinocchio-v3

For now, there is a "DEPEND_ABI.python+= python<3" in py-pinocchio/Makefile,
so we don't have to fear for any conflict… So would you mind if we put those
headers and the .pc back for a while ?

Thanks,
Guilhem.

---Files--------------------------------
signature.asc (833 Bytes)
0001-math-py-pinocchio-Force-py-pinocchio-to-python-2.7-t.patch (2.57 KB)

This is not a pure python package.
It provides bindings between a C++ library (pinocchio)and python.
Thus it should be pinocchio to provide the headers.
It used to be the case in wip.

OK, but shouldn't those headers be private to the py-pinocchio
package, then? Why do other packages need to access the headers? Cannot the
other packages use py-pinocchio?

I think it is indeed bad, and the proper way is to fix the PLIST of
pinocchio.

This raises another even worse issue : stricly speaking, if pinocchio
installs the python headers, the pinocchio package itself should then
depend on at least boost-python and py-eigenpy (because the headers
installed in python/bindings include those). Which comes back to the
first issue : this will prevent multiple python version to be used for
anything that depends on pinocchio.

Of course, not adding the python dependency on pinocchio will
technically work, because the headers are just installed and pinocchio
itself doesn't use them. But users might miss dependencies if they
start use headers in python/bindings ...

On 02/05/2018 14:22, Anthony Mallet wrote:

Issue #158 has been updated by Anthony Mallet.

This is not a pure python package.
It provides bindings between a C++ library (pinocchio)and python.
Thus it should be pinocchio to provide the headers.
It used to be the case in wip.

OK, but shouldn't those headers be private to the py-pinocchio
package, then? Why do other packages need to access the headers? Cannot the
other packages use py-pinocchio?

That is a very good question. I was also puzzled.

But after looking at the code, it makes senses.
The other library creates new C++ code based on pinocchio objects
AND new python bindings based on its own C++ objects and the ones from
pinocchio.
Therefore it needs the  data structures provided by the pinocchio bindings.

I think it is indeed bad, and the proper way is to fix the PLIST of
pinocchio.

This raises another even worse issue : stricly speaking, if pinocchio
installs the python headers, the pinocchio package itself should then
depend on at least boost-python and py-eigenpy (because the headers
installed in python/bindings include those). Which comes back to the
first issue : this will prevent multiple python version to be used for
anything that depends on pinocchio.

I think I found the problem on py-eigenpy which prevented to use
multiple python interpreters
with libboost-python.

It is currently being fix upstream in the main repo of jrl-cmakemodules
used for eigenpy
See
https://github.com/stack-of-tasks/eigenpy/commit/80a2aff29c3c794937d6714ff02f176478806f8c

The same patch could apply to pinocchio.

So if eigenpy and pinocchio are able to handle multiple python
interpreters together with libboost_python
I guess there is no problem, or do I miss another issue ?

Of course, not adding the python dependency on pinocchio will
technically work, because the headers are just installed and pinocchio
itself doesn't use them. But users might miss dependencies if they
start use headers in python/bindings ...

----------------------------------------
Bug #158: Re: Updating HPP in Robotpkg
https://git.openrobots.org/issues/158#change-273

• Author: Guilhem Saurel
• Status: New
• Priority: Normal
• Assignee:
• Category:
• Target version:
  ----------------------------------------
  Mail de Anthony Mallet du jeudi 2018-04-26 19:14:

« at the moment, the headers provided by the python bindings are not
installed because they would conflict between multiple python version. The .pc
file is not installed either for the same reason. If any of those files are
actually needed, one should figure out a proper namespace or a .pc
name that do not conflict. »

/local/users/gsaurel/robotpkg/robotpkg/wip/sot-dynamic-pinocchio-v3/work.akasan/sot-dynamic-pinocchio-v3-3.1.7/src/python-module-py.cpp:21:57: fatal error: pinocchio/bindings/python/multibody/model.hpp: No such file or directory

So this is breaking sot-dynamic-pinocchio-v3

For now, there is a "DEPEND_ABI.python+= python<3" in py-pinocchio/Makefile,
so we don't have to fear for any conflict… So would you mind if we put those
headers and the .pc back for a while ?

Thanks,
Guilhem.

---Files--------------------------------
0001-math-py-pinocchio-Force-py-pinocchio-to-python-2.7-t.patch (2.57 KB)

On Wednesday 2 May 2018, at 14:48, Olivier Stasse wrote:

But after looking at the code, it makes senses. The other library
creates new C++ code based on pinocchio objects AND new python
bindings based on its own C++ objects and the ones from pinocchio.
Therefore it needs the  data structures provided by the pinocchio
bindings.

OK, got it.

https://github.com/stack-of-tasks/eigenpy/commit/80a2aff29c3c794937d6714ff02f176478806f8c

The same patch could apply to pinocchio.

Yes, but this specific patch has to be applied to py-pinocchio
only. See why below.

So if eigenpy and pinocchio are able to handle multiple python
interpreters together with libboost_python I guess there is no
problem, or do I miss another issue ?

Yes, you miss this:

• Suppose you want to install py35-pinocchio and py27-pinocchio.
• Both depend on pinocchio.
• If pinocchio depends on python, pinocchio will be built with one
  python, say 2.7.
• This will prevent py35-pinocchio from installing, because
  py35-pinocchio wants python35 and pinocchio wants python25, and a
  chain of dependencies must use only one python interpreter in
  robotpkg.

Now, if pinocchio does not depend on python, problem solved.

So, what I propose is either a/ or b/:

a/

• install python/bindings headers in the pinocchio package.
• and do not add the dependency on python to pinocchio. The installed
  headers will miss the python dependency and it is up to each
  py-*package* wanting to make use of the headers to add the
  dependency on python.

b/

• install python/bindings headers in the py[0-9][0-9]-pinocchio package
• use a path such as
  include/pinocchio/python27/bindings/xxx.h, or
  include/pinocchio/python35/bindings/xxx.h for the headers,
  so that multiple py[0-9][0-9]-pinocchio can be installed
  simultaneously.
• use a name like pinocchio-py27.pc, pinocchio-py35.pc etc. for the
  .pc file

Pick up the one that you prefer and I'll implement it :)

On 02/05/2018 15:39, Anthony Mallet wrote:

Issue #158 has been updated by Anthony Mallet.

On Wednesday 2 May 2018, at 14:48, Olivier Stasse wrote:

But after looking at the code, it makes senses. The other library
creates new C++ code based on pinocchio objects AND new python
bindings based on its own C++ objects and the ones from pinocchio.
Therefore it needs the  data structures provided by the pinocchio
bindings.

OK, got it.

Well it appears after a deeper look that the headers are useless
in this specific code. Thus I am fixing it upstream.

Therefore before asking you to implement one or the other solution
you nicely proposed, we will check the other packages and see if there
is really problem.

Meanwhile we are preparing a release for eigenpy to allow multiple py
interpreters.

Thanks again for your support and your suggestions.

All the best,
Olivier.

https://github.com/stack-of-tasks/eigenpy/commit/80a2aff29c3c794937d6714ff02f176478806f8c

The same patch could apply to pinocchio.

Yes, but this specific patch has to be applied to py-pinocchio
only. See why below.

So if eigenpy and pinocchio are able to handle multiple python
interpreters together with libboost_python I guess there is no
problem, or do I miss another issue ?

Yes, you miss this:

• Suppose you want to install py35-pinocchio and py27-pinocchio.
• Both depend on pinocchio.
• If pinocchio depends on python, pinocchio will be built with one
  python, say 2.7.
• This will prevent py35-pinocchio from installing, because
  py35-pinocchio wants python35 and pinocchio wants python25, and a
  chain of dependencies must use only one python interpreter in
  robotpkg.

Now, if pinocchio does not depend on python, problem solved.

So, what I propose is either a/ or b/:

a/

• install python/bindings headers in the pinocchio package.
• and do not add the dependency on python to pinocchio. The installed
  headers will miss the python dependency and it is up to each
  py-*package* wanting to make use of the headers to add the
  dependency on python.

b/

• install python/bindings headers in the py[0-9][0-9]-pinocchio package
• use a path such as
  include/pinocchio/python27/bindings/xxx.h, or
  include/pinocchio/python35/bindings/xxx.h for the headers,
  so that multiple py[0-9][0-9]-pinocchio can be installed
  simultaneously.
• use a name like pinocchio-py27.pc, pinocchio-py35.pc etc. for the
  .pc file

Pick up the one that you prefer and I'll implement it :)

----------------------------------------
Bug #158: Re: Updating HPP in Robotpkg
https://git.openrobots.org/issues/158#change-275

• Author: Guilhem Saurel
• Status: New
• Priority: Normal
• Assignee:
• Category:
• Target version:
  ----------------------------------------
  Mail de Anthony Mallet du jeudi 2018-04-26 19:14:

« at the moment, the headers provided by the python bindings are not
installed because they would conflict between multiple python version. The .pc
file is not installed either for the same reason. If any of those files are
actually needed, one should figure out a proper namespace or a .pc
name that do not conflict. »

/local/users/gsaurel/robotpkg/robotpkg/wip/sot-dynamic-pinocchio-v3/work.akasan/sot-dynamic-pinocchio-v3-3.1.7/src/python-module-py.cpp:21:57: fatal error: pinocchio/bindings/python/multibody/model.hpp: No such file or directory

So this is breaking sot-dynamic-pinocchio-v3

For now, there is a "DEPEND_ABI.python+= python<3" in py-pinocchio/Makefile,
so we don't have to fear for any conflict… So would you mind if we put those
headers and the .pc back for a while ?

Thanks,
Guilhem.

---Files--------------------------------
0001-math-py-pinocchio-Force-py-pinocchio-to-python-2.7-t.patch (2.57 KB)

Dear Anthony,
I reverted my patch on py-pinocchio and release a new version of eigenpy as well as sot-dynamic-pinocchio.
The later one is now compiling which was the hard point for us.

However the following points are remaining:

1/ The system is trying to get boost_python 3_5 on Ubuntu 14.04 LTS.
It is probably due to a remaining line 
SET in the CMakeLists.txt
Reading the CMake Macro
https://github.com/Kitware/CMake/blob/master/Modules/FindPythonLibs.cmake#L113
I thought that the alternative version would be added instead of being the only alternative.
Do you have any idea about this problem ?

2/ py-pinocchio is compiled only for 2.7 which is due to the 
DEPEND_ABI.python+=    python<3
https://git.openrobots.org/projects/robotpkg/repository/revisions/master/entry/math/py-pinocchio/Makefile#L40

So fixing 2 is easy, and if you have an idea on fixing 1, I guess the issue could be closed.

Best,
Olivier.

On Thursday 3 May 2018, at 09:14, Olivier Stasse wrote:

1/ The system is trying to get boost_python 3_5 on Ubuntu 14.04 LTS.
It is probably due to a remaining line 
SET in the CMakeLists.txt Reading
the CMake Macro
https://github.com/Kitware/CMake/blob/master/Modules/FindPythonLibs.cmake#L113
I thought that the alternative version would be added instead of
being the only alternative. Do you have any idea about this problem
?

Yes, this was fixed in the py-eigenpy with the two patch-a{c,d} that I
mentionned earlier in this thread. But you removed them with no reason
(or at least no log message), so now they are missing ...

2/ py-pinocchio is compiled only for 2.7 which is due to the 
DEPEND_ABI.python+=    python<3
https://git.openrobots.org/projects/robotpkg/repository/revisions/master/entry/math/py-pinocchio/Makefile#L40

This is because the CMakeList of the package does restrict itself to
python<3, so the line above is here to indicate this to robotpkg.

For py-eigenpy it was discussed in private e-mails with Guilhem
whether allowing python3 was appropriate or not, but I did not do that
yet for py-pinocchio as there were bigger issues (this one) to be
sorted out first.

On 03/05/2018 11:48, Anthony Mallet wrote:

Issue #158 has been updated by Anthony Mallet.

On Thursday 3 May 2018, at 09:14, Olivier Stasse wrote:

1/ The system is trying to get boost_python 3_5 on Ubuntu 14.04 LTS.
It is probably due to a remaining line 
SET in the CMakeLists.txt Reading
the CMake Macro
https://github.com/Kitware/CMake/blob/master/Modules/FindPythonLibs.cmake#L113
I thought that the alternative version would be added instead of
being the only alternative. Do you have any idea about this problem
?

Yes, this was fixed in the py-eigenpy with the two patch-a{c,d} that I
mentionned earlier in this thread. But you removed them with no reason
(or at least no log message), so now they are missing ...

My bad, I thought that the fixed pushed in the jrl-cmakemodules that you
are patching was
equivalent to patch-ad.
Or at least the tests I did let me think so.
But it seems that this is not the case.
Anyway, in the long term your patches should be used upstream.

2/ py-pinocchio is compiled only for 2.7 which is due to the 
DEPEND_ABI.python+=    python<3
https://git.openrobots.org/projects/robotpkg/repository/revisions/master/entry/math/py-pinocchio/Makefile#L40

This is because the CMakeList of the package does restrict itself to
python<3, so the line above is here to indicate this to robotpkg.

For py-eigenpy it was discussed in private e-mails with Guilhem
whether allowing python3 was appropriate or not, but I did not do that
yet for py-pinocchio as there were bigger issues (this one) to be
sorted out first.

----------------------------------------
Bug #158: Re: Updating HPP in Robotpkg
https://git.openrobots.org/issues/158#change-278

• Author: Guilhem Saurel
• Status: New
• Priority: Normal
• Assignee:
• Category:
• Target version:
  ----------------------------------------
  Mail de Anthony Mallet du jeudi 2018-04-26 19:14:

« at the moment, the headers provided by the python bindings are not
installed because they would conflict between multiple python version. The .pc
file is not installed either for the same reason. If any of those files are
actually needed, one should figure out a proper namespace or a .pc
name that do not conflict. »

/local/users/gsaurel/robotpkg/robotpkg/wip/sot-dynamic-pinocchio-v3/work.akasan/sot-dynamic-pinocchio-v3-3.1.7/src/python-module-py.cpp:21:57: fatal error: pinocchio/bindings/python/multibody/model.hpp: No such file or directory

So this is breaking sot-dynamic-pinocchio-v3

For now, there is a "DEPEND_ABI.python+= python<3" in py-pinocchio/Makefile,
so we don't have to fear for any conflict… So would you mind if we put those
headers and the .pc back for a while ?

Thanks,
Guilhem.

---Files--------------------------------
0001-math-py-pinocchio-Force-py-pinocchio-to-python-2.7-t.patch (2.57 KB)