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

Choose collection based on environment variable #72

Merged
merged 6 commits into from
Jul 22, 2020

Conversation

chapulina
Copy link
Contributor

Since #48, we support both Blueprint and Citadel at the CMake level, but package.xml has only been supporting Citadel.

Then on #71, a couple of package-level dependencies were reverted to Blueprint to get a release out.

This pull request leverages the condition (see REP 149) argument to support both collections based on the IGNITION_VERSION environment variable, defaulting to Citadel when the variable is not present.

Signed-off-by: Louise Poubel <louise@openrobotics.org>
ros_ign_gazebo/package.xml Outdated Show resolved Hide resolved
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
Copy link
Contributor Author

@j-rivero , I think this will break blooming Blueprint-compatible packages unless we expose the Ignition version to release-tools/bloom/release-bloom.py. It shouldn't be as simple as setting the $IGNITION_VERSION variable before calling that script, I think. The dependencies are fetched inside the Jenkins node, right?

@j-rivero
Copy link
Contributor

It shouldn't be as simple as setting the $IGNITION_VERSION variable before calling that script, I think.

If I'm not wrong, the rosdep keys are resolved at the time of running bloom which usually happens in the developer local system. We need to integrate the $IGNITION_VERSION in our releasing tools but probably it does not affect to Jenkins scripts since at that time the only thing used are debian package names generated by bloom.

@j-rivero
Copy link
Contributor

defaulting to Citadel when the variable is not present.

If IGNITION_VERSION is not defined, my system is trying to get Blueprint dependencies (see colcon log about ignition-gazebo2) instead of defaulting to Citadel. Am I doing something wrong?

jrivero@nium ros_ign_ws $ unset IGNITION_VERSION &&  MAKEJOBS="-j1" colcon build --executor sequential --event-handler console_direct+  | grep ignition-gazebo2
CMake Error at CMakeLists.txt:26 (find_package):
  By not providing "Findignition-gazebo2.cmake" in CMAKE_MODULE_PATH this
  project has asked CMake to find a package configuration file provided by
  "ignition-gazebo2", but CMake did not find one.

@chapulina
Copy link
Contributor Author

If IGNITION_VERSION is not defined, my system is trying to get Blueprint dependencies

That error is coming in the CMake level, not the package level. The CMake code is not aware of the new env var and will always try Citadel first. Maybe we should change that 💡


The way I tested this PR with colcon was:

  1. Drop ros_ign in a workspace with other Ignition libs, i.e.
src/
|-- ign-cmake
|-- ign-common
|-- ign-fuel-tools
|-- ign-gazebo
|-- ign-gui
|-- ign-launch
|-- ign-math
|-- ign-msgs
|-- ign-physics
|-- ign-plugin
|-- ign-rendering
|-- ign-sensors
|-- ign-tools
|-- ign-transport
|-- ros_ign 
`-- sdformat
  1. Use colcon graph to check the inter-dependencies inside the workspace:

colcon graph --dot | grep "ros_ign_.*->.*ignition"

If you have blueprint dependencies in the workspace and IGNITION_VERSION=blueprint:

  "ros_ign_gazebo_demos" -> "ignition-gazebo2" [color="#0000ff:#ff0000"];
  "ros_ign_gazebo" -> "ignition-gazebo2" [color="#0000ff:#ff0000"];
  "ros_ign_image" -> "ignition-transport7" [color="#0000ff:#ff0000"];
  "ros_ign_image" -> "ignition-msgs4" [color="#0000ff:#ff0000"];
  "ros_ign_bridge" -> "ignition-transport7" [color="#0000ff:#ff0000"];
  "ros_ign_bridge" -> "ignition-msgs4" [color="#0000ff:#ff0000"];

If the variable is anything other than blueprint, the command won't show anything.

You can also try the other way around in a citadel workspace.

Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina chapulina requested a review from caguero as a code owner July 21, 2020 21:29
@chapulina
Copy link
Contributor Author

@j-rivero , I updated the CMake code to also check the environment variables.

I also went ahead and tackled #90 while documenting the IGNITION_VERSION variable.

This is ready for review again.

Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina chapulina added ROS 1 ROS 1 🏰 citadel Ignition Citadel 📜 blueprint Ignition Blueprint labels Jul 22, 2020
@j-rivero
Copy link
Contributor

Code looks good, compilation against Blueprint and Citadel works as expected and a quick smoke test seems to be able to load things in memory. Nice work.

@j-rivero j-rivero merged commit a3d07cd into melodic Jul 22, 2020
@j-rivero j-rivero deleted the conditional_collection branch July 22, 2020 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📜 blueprint Ignition Blueprint 🏰 citadel Ignition Citadel ROS 1 ROS 1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants