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

CI: SITL test run: select ROS version based on ROS_DISTRO #11338

Merged
merged 1 commit into from
Mar 9, 2019

Conversation

lamping7
Copy link
Member

As we introduce SITL testing on Melodic this is important as we'll have Kinetic at the same time.

TSC21
TSC21 previously approved these changes Jan 29, 2019
@lamping7
Copy link
Member Author

@lamping7 lamping7 changed the title CI: SITL test run: select correct ROS version [WIP] CI: SITL test run: select correct ROS version Jan 30, 2019
@lamping7
Copy link
Member Author

lamping7 commented Mar 8, 2019

Working on updating images to support this --> #11606

@lamping7
Copy link
Member Author

lamping7 commented Mar 9, 2019

rebased

test/rostest_px4_run.sh Outdated Show resolved Hide resolved
@lamping7 lamping7 force-pushed the pr-ci_test_ros_distro branch from 9983cb9 to 4f2a8e3 Compare March 9, 2019 12:25
@lamping7 lamping7 changed the title [WIP] CI: SITL test run: select correct ROS version CI: SITL test run: select ROS version based on ROS_DISTRO Mar 9, 2019
@lamping7 lamping7 requested review from dagar and TSC21 March 9, 2019 12:41
Copy link
Member

@TSC21 TSC21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not set the distro based on the OS distro?

@lamping7
Copy link
Member Author

lamping7 commented Mar 9, 2019

Why not set the distro based on the OS distro?

This is a good idea, but I see issues with it. It would be fairly trivial if we assume this will only be used with Ubuntu. We could run something like lsb_release -sc and set it based on that. The problem is the assumption. To actually test for the users OS and set this appropriately, we need to consider Debian, Arch, and so on. I don't want to claim we're checking the distro and don't actually check for all the flavors compatible with Kinetic and Melodic respectively. So, I'd rather just fall back to the default ROS version supported in our CI system.

@TSC21
Copy link
Member

TSC21 commented Mar 9, 2019

@lamping7
Copy link
Member Author

lamping7 commented Mar 9, 2019

Check this example: https://github.com/PX4/px4_ros_com/blob/master/scripts/setup_system.bash

That's exactly what I said we could do. I don't want to assume that everyone only uses Ubuntu.

@TSC21
Copy link
Member

TSC21 commented Mar 9, 2019

Check this example: https://github.com/PX4/px4_ros_com/blob/master/scripts/setup_system.bash

That's exactly what I said we could do. I don't want to assume that everyone only uses Ubuntu.

ROS officially supports Ubuntu and Debian. Both Ubuntu and Debian support the lsb-release commands. The support for Arch, Gentoo, etc. for ROS is experimental, though it's also possible to verify the release version of them. In the end, I think what matters is what platforms we want to support based on where ROS can run (officially). Am I wrong to assume that?

@lamping7
Copy link
Member Author

lamping7 commented Mar 9, 2019

There's nothing wrong with your statement. It's just beyond the scope of what I was going for here. I was only trying to dynamically source the correct ROS setup for CI purposes as stated in the title and the original post. I still think the default matching what is officially used for testing is fine as it is only used for that purpose. If users want to run this on their own machine, they should have already sourced the correct setup script for their environment as defined in the installation instructions for ROS. If users run this inside the px4 docker container the environment variable will be set. I don't see any issues with this as is.

@dagar
Copy link
Member

dagar commented Mar 9, 2019

Good enough for now. We can continue to generalize further.

@dagar dagar merged commit a9fc04b into PX4:master Mar 9, 2019
@lamping7 lamping7 deleted the pr-ci_test_ros_distro branch March 10, 2019 03:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants