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

Planned upgrade to Edifice #306

Closed
FirefoxMetzger opened this issue Mar 26, 2021 · 15 comments · Fixed by #307
Closed

Planned upgrade to Edifice #306

FirefoxMetzger opened this issue Mar 26, 2021 · 15 comments · Fixed by #307

Comments

@FirefoxMetzger
Copy link
Contributor

Since Ignition Edifice is now released, I was wondering when the switch will occur for gym-ignition.

The reason I'm asking is that I'm trying to set up a dev build of gym-ignition, and I'm having trouble with protoc (google protocol buffers). The binary installation of ign-msg6 for Dome uses an older version (I need to figure out which one exactly) and supplies precompiled wrappers. This causes local compilation of gym-ignition to fail. Maybe switching to Edifice can solve this problem for me (haven't checked).

@traversaro
Copy link
Contributor

The reason I'm asking is that I'm trying to set up a dev build of gym-ignition, and I'm having trouble with protoc (google protocol buffers). The binary installation of ign-msg6 for Dome uses an older version (I need to figure out which one exactly) and supplies precompiled wrappers. This causes local compilation of gym-ignition to fail.

Interesting. Are you using apt ? On which distro? If ign-msgs6 have precompiled wrappers that are incompatible with the apt system-provided protobuf version, probably the issue is in Ignition deb packages.

@FirefoxMetzger
Copy link
Contributor Author

Are you using apt ? On which distro?

Yes. apt on Ubuntu 20.04.

It is indeed not a gym-ignition issue and in all likelihood an ign-msg6 issue, which is why I didn't open an issue here. I haven't tracked it down fully yet though, so I can't be certain. Once I know, I will flag it over at ign-msg6.

@diegoferigo
Copy link
Collaborator

diegoferigo commented Mar 26, 2021

Can you please provide the compilation error that occurred? We do not include anything related to protobuf, it's just a private transitive dependency of Ignition Gazebo. I just remembered that this is not true. In rare occasions, when Ignition APIs are missing, we process the class from ign-msgs (e.g. here).

Since Ignition Edifice is now released, I was wondering when the switch will occur for gym-ignition.

In any case, the next tagged release will support Edifice. At the moment we don't have interesting features in preparation, if there's nothing coming in the next weeks, I'll make a new release just to bump the supported Ignition distribution.

@FirefoxMetzger
Copy link
Contributor Author

FirefoxMetzger commented Mar 26, 2021

It's various ways of letting me know that my protobuf versions have a mismatch. It's pretty noisy since this fails individually for each known message.

/usr/include/ignition/msgs6/ignition/msgs/serialized.pb.h:17:2: error: #error This file was generated by an older version of protoc 
[...]
/usr/include/ignition/msgs6/ignition/msgs/serialized.pb.h:18:2: error: #error incompatible with your Protocol Buffer headers.
[...]
/usr/include/ignition/msgs6/ignition/msgs/entity.pb.h:19:2: error: #error regenerate this file with a newer version of protoc.
[...]

Here is the full build log (obtained via make >build.log 2>build.log): https://gist.github.com/FirefoxMetzger/9b0b3d3bec730df4048346b0ba63276e

Edit: I also use a binary build of Ignition Dome from apt.

@diegoferigo
Copy link
Collaborator

diegoferigo commented Mar 26, 2021

Can you check what are the protobuf resources that gym-ignition detects, and make sure that it's the same version used to build the Ignition binaries? Unfortunately protobuf always caused troubles to us (and, transitively, our downstream users). I guess that the mismatch could be solved with a source installation of Ignition Gazebo making sure that the detected protobuf matches the version detected by gym-ignition.

In my case I have the following, note that I'm testing a conda setup to provide all the dependencies, and then I use colcol to build Ignition from sources. This means that I always make the libraries find the conda's protobuf.

gym-ignition/build on devel [$!?] via v3.19.4 via 🅒 /conda 
❯ cat CMakeCache.txt | grep proto
Protobuf_LIBRARY_DEBUG:FILEPATH=/conda/lib/libprotobuf.so
Protobuf_LIBRARY_RELEASE:FILEPATH=/conda/lib/libprotobuf.so
Protobuf_LITE_LIBRARY_DEBUG:FILEPATH=/conda/lib/libprotobuf-lite.so
Protobuf_LITE_LIBRARY_RELEASE:FILEPATH=/conda/lib/libprotobuf-lite.so
Protobuf_PROTOC_EXECUTABLE:FILEPATH=/conda/bin/protoc
Protobuf_PROTOC_LIBRARY_DEBUG:FILEPATH=/conda/lib/libprotoc.so
Protobuf_PROTOC_LIBRARY_RELEASE:FILEPATH=/conda/lib/libprotoc.so
FIND_PACKAGE_MESSAGE_DETAILS_Protobuf:INTERNAL=[/conda/lib/libprotobuf.so;-lpthread][/conda/include][v3.14.0(3)]

@traversaro
Copy link
Contributor

I would also suggest to open /usr/include/ignition/msgs6/ignition/msgs/entity.pb.h:19:2: error: #error regenerate this file with a newer version of protoc. and check the protobuf version with which those headers were generated. As apt's 20.04 ships with 3.6.1.3 (see https://repology.org/project/protobuf/versions) if the version used to generate those headers is different I guess there is some problem in the .deb packages of ignition-msgs6, otherwise it the version matches 3.6.1.3, I would bet that there is some other non-system apt installed version of protobuf that is used installed in your system.

@FirefoxMetzger
Copy link
Contributor Author

FirefoxMetzger commented Mar 26, 2021

I would also suggest to open [...] and check the protobuf version with which those headers were generated.

Very true, the version should be mentioned somewhere in the file. Excellent suggestion, thanks!

It seems that I am indeed running v3.6.1.3 and that the files are also compiled with v3.6.1 😕. I guess there is a chance that there is something funny on my system, even though it is a VM that only contains Ignition Dome and gym-ignition ... I will see if there is another version and what cmake actually ends up finding.

logs
sebastian@DESKTOP-2SO47AE:~$ head -20 /usr/include/ignition/msgs6/ignition/msgs/entity.pb.h
// Generated by the protocol buffer compiler.  DO NOT EDIT!
// source: ignition/msgs/entity.proto

#ifndef PROTOBUF_INCLUDED_ignition_2fmsgs_2fentity_2eproto
#define PROTOBUF_INCLUDED_ignition_2fmsgs_2fentity_2eproto

#include <string>

#include <google/protobuf/stubs/common.h>

#if GOOGLE_PROTOBUF_VERSION < 3006001
#error This file was generated by a newer version of protoc which is
#error incompatible with your Protocol Buffer headers.  Please update
#error your headers.
#endif
#if 3006001 < GOOGLE_PROTOBUF_MIN_PROTOC_VERSION
#error This file was generated by an older version of protoc which is
#error incompatible with your Protocol Buffer headers.  Please
#error regenerate this file with a newer version of protoc.
#endif
sebastian@DESKTOP-2SO47AE:~$ protoc --version
libprotoc 3.6.1
sebastian@DESKTOP-2SO47AE:~$ dpkg-query -l | grep libprotoc
ii  libprotoc-dev:amd64                           3.6.1.3-2ubuntu5                      amd64        protocol buffers compiler library (development files)
ii  libprotoc17:amd64                             3.6.1.3-2ubuntu5                      amd64        protocol buffers compiler library

This does highlight an interesting point though: The protobuf messages enforce matching protobuf versions for major, minor, AND patch. I can see how the latter two can cause at least some problems, given that ign-msg6 seems to only enforce matching version major in their CMakeLists.

@diegoferigo diegoferigo linked a pull request Mar 26, 2021 that will close this issue
9 tasks
@FirefoxMetzger
Copy link
Contributor Author

FirefoxMetzger commented Mar 26, 2021

I managed to solve the protobuf issue 🚀 Thanks for your help and comments @diegoferigo @traversaro

For future reference: The problem was (as expected) a wrong protobuf version. My VM orchestration tool (WSL) mounts the host's file system into the Linux container under /mnt/c/. My Windows indeed uses a different protobuf version and - for reasons I have yet to fully understand - CMake was adamant in finding the Windows installation under /mnt/c/[...] and preferred it over the one installed under /usr/lib 💯

There is - at the time of this writing - no good solution to tell find_package to exclude a prefix (related issue over at CMake: https://gitlab.kitware.com/cmake/cmake/-/issues/20878). In my case, I excluded the projects explicitly via CMAKE_IGNORE_PATH (only works on absolute paths, non-recursive). I haven't found a more general solution short of removing the folder on the host yet.

@traversaro
Copy link
Contributor

My Windows indeed uses a different protobuf version and - for reasons I have yet to fully understand - CMake was adamant in finding the Windows installation under /mint/c/[...] and preferred it over the one installed under /usr/lib 💯

I am not sure, but in the past I had similar problem and almost always they were due to the PATH environment variable that even in WSL contained the directories of the Windows system. The two possible solutions are:

  • Sanify the PATH or any other env variable from path that start with /mnt/c/ in source/setup/.bashrc script
  • Disable at all the behavior of having Windows directory in the WSL PATH, however that in this case code may not work as expected (I usually solve that with a custom alias).

You can find more info in the robotology-superbuild docs:

@diegoferigo
Copy link
Collaborator

For those having a local installation of Edifice, I believe the following should work to get a working setup with gym-ignition:

pip install git+https://github.com/robotology/gym-ignition@feature/edifice

Preview of #307. I'll be testing Edifice locally for a while before merging the PR into the Nightly channel. I don't expect major problems since there are no breaking changes from Dome (apart from some deprecation), however early feedback is appreciated.

@traversaro
Copy link
Contributor

Since Ignition Edifice is now released, I was wondering when the switch will occur for gym-ignition.

Just to clarify, I am afraid that the official Ignition Docs is a bit misleading, even if Ignition Edifice is going to be released soon as of today Ignition Edifice has not been released, i.e. there is no official "release" tag for Ignition Gazebo 5 and for all the projects that will be part of Ignition Edifice (see https://github.com/ignitionrobotics/ign-gazebo/releases).

@diegoferigo
Copy link
Collaborator

diegoferigo commented Mar 29, 2021

Are you sure that binaries did not yet land into the pre-release channel? I guess that while the tags are not yet there, one can use colcon to install Edifice and (maybe) also the binaries if they have been already uploaded. I personally tested Edifice with colcon. I used the released word that in this moment is not correct, though apart from this subtlety everything else is valid.

@traversaro
Copy link
Contributor

Are you sure that binaries did not yet land into the pre-release channel? I guess that while the tags are not yet there, one can use colcon to install Edifice and (maybe) also the binaries if they have been already uploaded. I personally tested Edifice with colcon. I used the released word that in this moment is not correct, though apart from this subtlety everything else is valid.

Yes, everything else is valid, also pre-release binaries are available etc etc. I just pointed out this as I think it could make sense to wait for a release (in the full sense, let's say) before doing a release of gym-ignition that targets Ignition Edifice.

@diegoferigo
Copy link
Collaborator

diegoferigo commented Mar 29, 2021

Oh yes that was implicit :) In any case, #307 targets devel, and in theory our policy allows using unreleased Ignition distributions.

@diegoferigo
Copy link
Collaborator

The nightly channel now supports Edifice 🚀 Closed via #307.

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 a pull request may close this issue.

3 participants