traversaro
commented
3 years ago Probably something strange is going on at https://github.com/ignitionrobotics/ign-common/blob/33bde9a916a578566288584d05033eec8e3015b5/src/SystemPaths.cc#L163 .
Closed traversaro closed 3 years ago
traversaro
commented
3 years ago Probably something strange is going on at https://github.com/ignitionrobotics/ign-common/blob/33bde9a916a578566288584d05033eec8e3015b5/src/SystemPaths.cc#L163 .
diegoferigo
commented
3 years ago Is it possible that the OS dependent logic used in Ignition (likely ign-plugin or ign-common) to form the shared library name is broken in conda? From the error, it searches for:
<env>/lib/ign-gazebo-4/plugins/ignition-gazebo-physics-systemwhen, on Linux, it should search instead:
<env>/lib/ign-gazebo-4/plugins/libignition-gazebo-physics-system.soIs it possible that the OS dependent logic used in Ignition (likely ign-plugin or ign-common) to form the shared library name is broken in conda? From the error, it searches for:
<env>/lib/ign-gazebo-4/plugins/ignition-gazebo-physics-systemwhen, on Linux, it should search instead:
<env>/lib/ign-gazebo-4/plugins/libignition-gazebo-physics-system.so
Yes, it seems something like that, I wonder if the binary relocation logic in conda is doing some dark magic.
diegoferigo
commented
3 years ago I had a look to this problem. It seems that the default plugin path added in SystemLoader.cc#L58:
std::string homePath;
ignition::common::env(IGN_HOMEDIR, homePath);
systemPaths.AddPluginPaths(homePath + "/.ignition/gazebo/plugins");
systemPaths.AddPluginPaths(IGN_GAZEBO_PLUGIN_INSTALL_DIR); // <-- This lineuses the macro IGN_GAZEBO_PLUGIN_INSTALL_DIR defined in config.hh.in#L19. For some reason I haven't yet understood, a lot of \0 characters are appended, messing up the logic used in ignition common to build the candidates for the library name.
I didn't get the origin of the problem, but the following provides a quick fix:
diff --git a/src/SystemPaths.cc b/src/SystemPaths.cc
index ff9c0c2..d87c9ed 100644
--- a/src/SystemPaths.cc
+++ b/src/SystemPaths.cc
@@ -235,7 +235,9 @@ void SystemPaths::AddFilePaths(const std::string &_path)
std::string SystemPaths::NormalizeDirectoryPath(const std::string &_path)
{
std::string path = _path;
- // Use '/' because it works on Linux, OSX, and Windows
+ path.erase(std::find(path.begin(), path.end(), '\0'), path.end());
+
+ // Use '/' because it works on Linux, OSX, and Windows
std::replace(path.begin(), path.end(), '\\', '/');
// Make last character '/'
if (!EndsWith(path, "/"))Thanks @diegoferigo . This is probably due to some interaction of the text/binary relocation feature of conda (see https://docs.conda.io/projects/conda-build/en/latest/resources/make-relocatable.html) and the line systemPaths.AddPluginPaths(IGN_GAZEBO_PLUGIN_INSTALL_DIR); . This is quite a conda-specific problem, but if that patch works I think it is worth to just add it as a patch in the recipe. However, how did you tested the patch? If you just compile ignition-gazebo from source, that should work fine even without the patch.
diegoferigo
commented
3 years ago This is probably due to some interaction of the text/binary relocation feature of conda (see https://docs.conda.io/projects/conda-build/en/latest/resources/make-relocatable.html) and the line
systemPaths.AddPluginPaths(IGN_GAZEBO_PLUGIN_INSTALL_DIR);. This is quite a conda-specific problem, but if that patch works I think it is worth to just add it as a patch in the recipe.
Ow ok I had no idea that such workaround was in place in conda, good to know! Thinking about it, it makes totally sense. Though, if this is true as it seems, also other macros could be affected.
However, how did you tested the patch?
I worked on the same environment described in https://github.com/conda-forge/libignition-gazebo-feedstock/issues/6. On top of it, I compiled ign-common from sources (all dependecies are found in the environment) and used as install prefix the environment itself. This is quite hacky since the files of the libignition-commonX get overridden, but it was the quickest solution I reached.
If you just compile ignition-gazebo from source, that should work fine even without the patch.
I didn't try it on the environment just described, but the development environment I'm using daily in the past month is conda-based. In particular, I install all the dependencies I need in a conda environment, and then I build against them a complete colcon environment with either Dome or Edifice. This setup is not affected by the problem, so I can confirm that it only occurs when using the binaries distributed through conda-forge.
traversaro
commented
3 years ago However, how did you tested the patch?
I worked on the same environment described in #6. On top of it, I compiled
ign-commonfrom sources (all dependecies are found in the environment) and used as install prefix the environment itself. This is quite hacky since the files of thelibignition-commonXget overridden, but it was the quickest solution I reached.
I see, so you continue to use the Ignition Gazebo binaries from conda? I see, that make sense.
This is probably due to some interaction of the text/binary relocation feature of conda (see https://docs.conda.io/projects/conda-build/en/latest/resources/make-relocatable.html) and the line
systemPaths.AddPluginPaths(IGN_GAZEBO_PLUGIN_INSTALL_DIR);. This is quite a conda-specific problem, but if that patch works I think it is worth to just add it as a patch in the recipe.Ow ok I had no idea that such workaround was in place in conda, good to know! Thinking about it, it makes totally sense. Though, if this is true as it seems, also other macros could be affected.
Note that are not the macros that are corrupted (I just checked the config.hh file and it seems fine), what is corrupted with trailing zero is the constant text buffer of the libignition-gazebo binary that is created when the systemPaths.AddPluginPaths(IGN_GAZEBO_PLUGIN_INSTALL_DIR) line is compiled.
diegoferigo
commented
3 years ago However, how did you tested the patch?
I worked on the same environment described in #6. On top of it, I compiled
ign-commonfrom sources (all dependecies are found in the environment) and used as install prefix the environment itself. This is quite hacky since the files of thelibignition-commonXget overridden, but it was the quickest solution I reached.I see, so you continue to use the Ignition Gazebo binaries from conda? I see, that make sense.
Actually, no :) My daily setup is and remains based on colcon using conda for the dependencies. I'd like to switch to conda binaries when deploying to HPC since it would allow me to bypass the need of using docker / singularity, but the quick workarounds I mentioned is too dirty and I need also other workarounds for other projects not listed here. I'm following the status of conda-forge distribution, but I'm not blocked thanks to the existing containerization setup I'm currently using.
This is probably due to some interaction of the text/binary relocation feature of conda (see https://docs.conda.io/projects/conda-build/en/latest/resources/make-relocatable.html) and the line
systemPaths.AddPluginPaths(IGN_GAZEBO_PLUGIN_INSTALL_DIR);. This is quite a conda-specific problem, but if that patch works I think it is worth to just add it as a patch in the recipe.Ow ok I had no idea that such workaround was in place in conda, good to know! Thinking about it, it makes totally sense. Though, if this is true as it seems, also other macros could be affected.
Note that are not the macros that are corrupted (I just checked the
config.hhfile and it seems fine), what is corrupted with trailing zero is the constant text buffer of the libignition-gazebo binary that is created when thesystemPaths.AddPluginPaths(IGN_GAZEBO_PLUGIN_INSTALL_DIR)line is compiled.
Yep thanks for clarifying, it is what I meant.
traversaro
commented
3 years ago I see, so you continue to use the Ignition Gazebo binaries from conda? I see, that make sense.
Actually, no :)
I meant for this specific test. Because if you do not install ignition-gazebo from conda-forge and instead you compile it from source, it will work even without this fix.
diegoferigo
commented
3 years ago Now I got your point. Yes, for those reading this issue up to here, if you install from conda-forge all the libignition-* packages excluding libignition-gazebo4 (being careful to select the correct ogre version https://github.com/conda-forge/libignition-gazebo-feedstock/issues/6), and then build from sources ign-gazebo using as install prefix the conda environment, you should get a working setup.
Disclaimer: I only tested Ubuntu, no idea about the other platforms.
Edit: I want also to add that this setup does not match what you get from the official Ubuntu binaries, since conda-forge uses upstream dart that does not have all the fixes that the Open Robotic's fork has. However, excluding particular cases, your simulation will be ok. Check ignition-forks/dart for more details.
traversaro
commented
3 years ago Now I got your point. Yes, for those reading this issue up to here, if you install from conda-forge all the
libignition-*packages excludinglibignition-gazebo4(being careful to select the correct ogre version #6), and then build from sourcesign-gazebousing as install prefix the conda environment, you should get a working setup.Disclaimer: I only tested Ubuntu, no idea about the other platforms.
Ok, so the patch https://github.com/conda-forge/libignition-gazebo-feedstock/issues/4#issuecomment-824709977 was never tested with a Ignition Gazebo installed via conda binaries?
diegoferigo
commented
3 years ago Now I got your point. Yes, for those reading this issue up to here, if you install from conda-forge all the
libignition-*packages excludinglibignition-gazebo4(being careful to select the correct ogre version #6), and then build from sourcesign-gazebousing as install prefix the conda environment, you should get a working setup. Disclaimer: I only tested Ubuntu, no idea about the other platforms.Ok, so the patch #4 (comment) was never tested with a Ignition Gazebo installed via conda binaries?
Yes yes I tested it and it works. However, I would not recommend users to override files of libignition-commonX as I described in https://github.com/conda-forge/libignition-gazebo-feedstock/issues/4#issuecomment-825454865. It is definitely not a good practice, it was useful for debugging though. Using my quoted solution of this comment is much cleaner.
Issue:
Running
ign gazebo -sfails with the following error:Environment (
conda list):Details about
condaand system (conda info):