Support paths from macros extended to support binary relocation

[diegoferigo]

Checklist

• Used a personal fork of the feedstock to propose changes
• Bumped the build number (if the version is unchanged)
• Reset the build number to 0 (if the version changed)
• Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
• Ensured the license file is being packaged.

This PR should fix conda-forge/libignition-gazebo-feedstock#4 by applying the patch proposed in conda-forge/libignition-gazebo-feedstock#4 (comment). I only tested this patch in Ubuntu 20.04, therefore for the moment I restricted the application to this OS. In any case, other OSs are disabled in the gazebo recipe (conda-forge/libignition-gazebo-feedstock#2).

This is my first attempt to packaging in conda, please let me know if anything is missing.

[conda-forge-linter]

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

[diegoferigo]

@conda-forge-admin, please rerender

[github-actions]

Hi! This is the friendly automated conda-forge-webservice.
I tried to rerender for you, but it looks like there was nothing to do.

[diegoferigo]

Btw, I had to add these two build dependencies otherwise building the package locally in my setup (without using docker) was failing, even though the active conda environment had it (I suspect the process runs isolated in a new env). To be precise, my setup is already in a docker container, and running the docker script would require me to setup DIND.

[traversaro]

Good to know, but please remove them. The recipe should only contain the dependency required to build the recipe on the conda-forge CI, anything else is quite difficult to maintain.

[diegoferigo]

Done

[diegoferigo]

Also, locally I had to rebuild the ignition gazebo recipe in order to fix conda-forge/libignition-gazebo-feedstock#4, I'm not sure if bumping the build number is enough to trigger a rebuild.

[traversaro]

Also, locally I had to rebuild the ignition gazebo recipe in order to fix conda-forge/libignition-gazebo-feedstock#4, I'm not sure if bumping the build number is enough to trigger a rebuild.

Strange, in theory it should be sufficient to just install the new version of ignition-commons, as this modification is part of the compiler library, it should not be necessary to re-build ignition-gazebo.

[diegoferigo]

Also, locally I had to rebuild the ignition gazebo recipe in order to fix conda-forge/libignition-gazebo-feedstock#4, I'm not sure if bumping the build number is enough to trigger a rebuild.

Strange, in theory it should be sufficient to just install the new version of ignition-commons, as this modification is part of the compiler library, it should not be necessary to re-build ignition-gazebo.

I was expecting the same, but without rebuilding also gazebo, either using the ign gazebo command line or running the simulator from Python (using scenario) was segfaulting. I don't exclude there's something odd in my system, I'm not yet familiar with the process.

[traversaro]

I only tested this patch in Ubuntu 20.04, therefore for the moment I restricted the application to this OS. In any case, other OSs are disabled in the gazebo recipe (conda-forge/libignition-gazebo-feedstock#2).

Thanks for handling this! I suggest to apply the same patch also on Windows and macOS. On a well-formed string that has \0 only as last character, that line is not doing anything. While ignition-gazebo is just building on Linux for now, I think it is just safer to have the patch in place in ignition-common for whenever macOS/Windows support is added in ignition-gazebo, without the need to actually remember to manually enable the patch on Windows/macOS whenever ign-gazebo gets macOS/Windows support.

Furthermore, this library can be already used as it is on Windows and macOS, so I think it is worth to just apply the patch everywhere.

[diegoferigo]

Thanks @traversaro! I've just addressed your suggestions.

[Tobias-Fischer]

Is it worth opening a PR upstream with this patch, or is it really only conda specific?

[diegoferigo]

If our understanding of the problem is correct (conda-forge/libignition-gazebo-feedstock#4 (comment)), this is very specific to how conda pads the IGN_GAZEBO_PLUGIN_INSTALL_DIR definition in order to make the package relocatable. This problem would not likely occur in any other system.

[traversaro]

If our understanding of the problem is correct (conda-forge/libignition-gazebo-feedstock#4 (comment)), this is very specific to how conda pads the IGN_GAZEBO_PLUGIN_INSTALL_DIR definition in order to make the package relocatable. This problem would not likely occur in any other system.

Exactly. The proper solution would be indeed to fix how conda handles the IGN_GAZEBO_PLUGIN_INSTALL_DIR definition, or understand how the padded literal is converted to a std::string to fix that at this level.

[wolfv]

Hey, sorry for chiming in so late. There is an issue with binary relocation + std::string because most std::string implementations do "small buffer optimization" and there are certain expectations by the C++ compiler for the string to keep it's size.
Not sure if that's the issue here? We had to do something similar in RViz.

conda/conda-build#1674

[traversaro]

Hey, sorry for chiming in so late. There is an issue with binary relocation + std::string because most std::string implementations do "small buffer optimization" and there are certain expectations by the C++ compiler for the string to keep it's size.
Not sure if that's the issue here? We had to do something similar in RViz.

conda/conda-build#1674

Yes, that's it, even in this case this happens because there is a systemPaths.AddPluginPaths(IGN_GAZEBO_PLUGIN_INSTALL_DIR); call and systemPaths.AddPluginPaths takes in input a std::string .

[traversaro]

This is also related to conda/conda-build#2524 . In general, the best upstream solution would be to ensure that the package is actually properly relocatable, without relying on conda tricks for this.