Skip to content
This repository has been archived by the owner on Mar 17, 2021. It is now read-only.

make install script support ROS kinetic and melodic #761

Closed
wants to merge 1 commit into from

Conversation

baumanta
Copy link

I was investigating if we could run our avoidance simulation under Ubuntu 18.04 using ROS melodic. I got the setup to work just fine (PX4 fimware, SITL gazebo, avoidance). But in the process I saw that the install script for the basic Toolchain (PX4 SITL gazebo) does not support Ubuntu 18.04. I changed the script such that it can be called either under Ubuntu 16.04 or 18.04.

The script was checked using virtual box and bare installations of Ubuntu 16.04 and 18.04. I seems to work for both.

I also added the paths/sourcing to bashrc automatically.

@hamishwillee
Copy link
Collaborator

Thanks @baumanta - did you see #753 :-)
It was sitting there waiting for Nuno to review - which happened yesterday

Can you compare the scripts and see which works best? My changes also include some work in docs surrounding the script.

@hamishwillee
Copy link
Collaborator

Further to this, melodic only works on 18.04 (not 16.04) and my script also needs to be updated to support the latest version FastRTPS

@hamishwillee
Copy link
Collaborator

hamishwillee commented Mar 30, 2019

I merged this, with new script for ROS melodic. Any advice on how to improve it much appreciated! https://github.com/PX4/Devguide/blob/master/build_scripts/ubuntu_sim_ros_melodic.sh

@baumanta
Copy link
Author

baumanta commented Apr 1, 2019

@hamishwillee the script I provided determines the ubuntu version you have (16.04 or 18.04) and then automatically installs the correct toolchain (melodic for 18.04 or kinetic for 16.04). I would say this is easier than having two separate scripts, right?

@baumanta
Copy link
Author

baumanta commented Apr 1, 2019

I'm not sure about the fats RTPS stuff. I can just verify that my script correctly installs the regular SITL toolchain. After executing it, I was able to roslaunch px4 posix_sitl.launch under both OS also when cloning avoidance, the avoidance simulation ran correctly. You would have to check in with Nuno and maybe add the RTPS stuff needed to this script.

@hamishwillee
Copy link
Collaborator

Yes. Except that according to @TSC21 we want them to use melodic, which to me meant lets have old script for old version of docs and new script for new version.

Nuno, I'm very happy to replace my script with this one, just need to know what you would prefer.

@hamishwillee
Copy link
Collaborator

@baumanta You don't need to worry about the RTPS stuff - that is imported using a dependency script of this (now updated)

@baumanta
Copy link
Author

baumanta commented Apr 1, 2019

ahh ok, then use whichever you like better :) the RTPS install script could also just return a message if the OS version does not match, such that the RTPS part is not installed under 16.04 and we can still have a single script. Or we can make two script out of it. Personally, I would like it better in one, but if that is hard to do on the RTPS script side, I'm fine to have two as well.

else echo "$firmware_source" >> ~/.bashrc; fi
eval $firmware_source

ros_package='export ROS_PACKAGE_PATH=${ROS_PACKAGE_PATH}:/home/tanja/src/Firmware:/home/tanja/src/Firmware/Tools/sitl_gazebo'
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should have hardcoded paths here.

@TSC21
Copy link
Member

TSC21 commented Apr 1, 2019

@baumanta @hamishwillee bringing Fast-RTPS into ROS install scripts will only make sense if you also install ROS2. Otherwise, it's not worth to include it on scripts with the sole purpose of installing ROS(1) toolchain.
Also a remark that under px4_ros_com I do already have a script for this (though notice I am not installing Fast RTPS there - the main reason is that ROS2 has it's own Fast RTPS implementation, though it still misses the fastrtpsgen script we require from the Fast RTPS installation. I am trying to find a way of getting this into a system without having to install Fast RTPS entirely).

@TSC21
Copy link
Member

TSC21 commented Apr 1, 2019

of docs and new script for new version.

Nuno, I'm very happy to replace my script with this one, just need to know what you would prefer.

As far as I know, we are updating our toolchains to newer versions. Backwards compatibility is always welcomed, though we tend to drop support on older versions as a normal process.

@hamishwillee
Copy link
Collaborator

@TSC21 OK, so as you know FastRTPS was always installed in these scripts - it came as the "common dependency": https://raw.githubusercontent.com/PX4/Devguide/master/build_scripts/ubuntu_sim_common_deps.sh
All I did was update to 1.7.1.
I think you are saying that this shouldn't be included except in the ROS scripts - because it isn't needed without ROS (?) and that ROS2 should also be installed too, or there isn't much point (?)

Which brings me to point 2. I originally created these devguide install scripts because no one else was and I found it frustrating to type all the bits in.
But really these end user scripts should be maintained by the "ROS" team and tested in CI. That way they can be kept up to date, and regularly tested. I can then link to them in the dev guide.

Any advice on how we can get from where we are now, to that state would be appreciated.

@TSC21
Copy link
Member

TSC21 commented Apr 1, 2019

@TSC21 OK, so as you know FastRTPS was always installed in these scripts - it came as the "common dependency": https://raw.githubusercontent.com/PX4/Devguide/master/build_scripts/ubuntu_sim_common_deps.sh
All I did was update to 1.7.1.
I think you are saying that this shouldn't be included except in the ROS scripts - because it isn't needed without ROS (?) and that ROS2 should also be installed too, or there isn't much point (?)

No, not exactly. You need Fast-RTPS installed if you actually want to build PX4 with Fast-RTPS support and run non-ROS Fast-RTPS apps on the machine (as we have documented). What I am saying is that installing this in ROS(1) scripts doesn't really add anything, unless you want to use px4_ros_com, which in that case you need an update on the scripts to also install ROS2.

I am ok that we keep these scripts here on the Devguide, but needs a commitment, as you said, from everyone working with these frameworks and maintaining a code base.

@hamishwillee
Copy link
Collaborator

hamishwillee commented Apr 1, 2019

I am ok that we keep these scripts here on the Devguide

I'm not. If they aren't in CI then they aren't being tested properly. If they are in devguide I don't believe we will ever get that proper level of commitment.

And the fact that it took me 3 reads of this to "think" I understand your point of ROS1 vs ROS2 and RTPS means that it should not be me owning these.

@hamishwillee
Copy link
Collaborator

hamishwillee commented Apr 2, 2019

@TSC21 @baumanta OK. Firstly, lets address this particular PR. I think we should close it. The script(s) in master should reflect the current supported version of ROS on the current supported version of Ubuntu. Let's discuss what needs to happen with scripts going forward in #764

The reason I feel this way is that we now have a versioned devguide. That means there is a snapshot of the devguide (including scripts) in a branch. So users working v1.8 should be offered whatever ROS was flavour of the month when the snapshot was taken (ROS Kinetic). In v1.9, users will be offered ROS Melodic.

Now I appreciate that v1.8 will support ROS melodic too, but the docs won't match up with that, and there is no way to be sure or test that. So we're better off offering what we know works for each version. Make sense?

This does mean I will have to rename my script back to something generic.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants