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

Add turtlebot-demo jenkins job. #35

Merged
merged 11 commits into from Mar 15, 2017
Merged

Add turtlebot-demo jenkins job. #35

merged 11 commits into from Mar 15, 2017

Conversation

nuclearsandwich
Copy link
Member

@nuclearsandwich nuclearsandwich commented Mar 14, 2017

First crack at implementing the plan described in #32 (comment)

This adds a new job to the Jenkins config by adding turtlebot-demo as an additional os_config. I looked for another place like job_configs to do this since the turtlebot isn't a separate os per se but it looks like os_configs is where this happens.

The job script will build a docker image with the INSTALL_TURTLEBOT2_DEMO_DEPS variable set for turtlebot-demo jobs.

Then dockerfile has an added directive to install the demo dependencies when that variable exists.

Still to do

cc @dhood and @tfoote for feedback and review.

This adds a new job to the Jenkins config by adding turtlebot-demo as an
additional os_config. I looked for another place like `job_configs` to
do this since the turtlebot isn't a separate os per se but it looks like
`os_configs` is where this happens.

The job script will build a docker image with the
`INSTALL_TURTLEBOT2_DEMO_DEPS` variable set for turtlebot-demo jobs.

Then dockerfile has an added directive to install the demo dependencies
when that variable exists.

Still to do

- [ ] verify what other system package dependencies should be installed
- [ ] create a repos file (at https://raw.githubusercontent.com/ros2/turtlebot2_demo/master/turtlebot2_demo.repos?)
- [ ] run this in a non-dryrun capacity to test the job
@nuclearsandwich nuclearsandwich added the in progress Actively being worked on (Kanban column) label Mar 14, 2017
Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

I think this will work, though I do feel it might be abusing the os_configs.

I would have thought it should have used a job_config and either reused the ci_job.xml.em or copy and customized it for the purposes of the new job. Specifically this would allow you to add OS X if you wanted to do so fairly easily. The current strategy doesn't scale in that direction as well, imo.

This could serve as a short term solution, but long term I think you would want to create a new class of job which is orthogonal to which os it is run on for this. Either way, thanks for looking into it!

@nuclearsandwich
Copy link
Member Author

I think this will work, though I do feel it might be abusing the os_configs.

I am in complete agreement, I looked (admittedly so far only within this script) for a larger global job_configs structure and didn't find it. My impetus for wanting to use an existing declarative structure rather than copy the loop contents separately was to avoid creating two separate invocations that need to be maintained.

It seems a bit ambitions but I suppose that the common job creation elements could be factored into functions to avoid strict duplication.

This pulls the turtlebot job out into its own section outside the
os jobs loop. It's possible down the line that this may be run within
the OS configs loop as another job. (ci_windows-turtlebot
ci_linux-turtlebot ...) but if that does happen we'll need to make it
easier for job config values to inject docker build params as right now
we use the same conditional block that switches the docker build
invocation based on os_name.

I think that refactoring is worth doing but it's something that can wait
to be paid down until we actually want this running on multiple
platforms.
@nuclearsandwich
Copy link
Member Author

create a new class of job which is orthogonal to which os it is run on for this. Either way, thanks for looking into it!

I've moved the job outside of the os_configs reliance but it's not yet on par with the other jobs as I'm not sure if we should be creating turtlebot jobs for all platforms. If we do want to do that right now, then we'll need to identify and install dependencies on the Windows and Mac Jenkins nodes. Additionally, we'll need to inject build-args into the docker build invocation a little more intelligently as right now the turtlebot job overrides the os_name and uses the standard linux invocation with one additional build-arg.

Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

This lgtm now.

If we do want to do that right now, then we'll need to identify and install dependencies on the Windows and Mac Jenkins nodes.

I think it's fine to leave it only on Linux for now.

Additionally, we'll need to inject build-args into the docker build invocation a little more intelligently as right now the turtlebot job overrides the os_name and uses the standard linux invocation with one additional build-arg.

A more involved solution would be to make the Dockerfile a template, template it differently for the normal jobs and the turtlebot jobs and then invoke the expanded Dockerfile template from the shell in the ci_job.xml.em file. But passing in the argument like this gets the job done for now.

Copy link
Member

@dhood dhood left a comment

Choose a reason for hiding this comment

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

this looks good. You can feel free to test non-dryrun when you want, as this should not affect the existing jobs, just add a new one.

@@ -176,7 +176,9 @@ echo "# BEGIN SECTION: docker info"
docker info
echo "# END SECTION"
echo "# BEGIN SECTION: Build Dockerfile"
@[if os_name == 'linux-armhf']@
@[if turtlebot_demo]@
Copy link
Member

@dhood dhood Mar 14, 2017

Choose a reason for hiding this comment

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

Rather than add another branch here, you can use the existing os_name=='linux' invocation and just specify the INSTALL_TURTLEBOT2_DEMO_DEPS arg with something like:

@[elif os_name == 'linux']@
docker build @(--build-arg INSTALL_TURTLEBOT2_DEMO_DEPS=yes if turtlebot_demo) -t ros2_batch_ci_turtlebot_demo linux_docker_resources
@[else]@

**edit:** removed extraneous quotes

@@ -38,6 +38,9 @@ RUN apt-get update && apt-get install -y build-essential ccache cmake pkg-config
# Install build and test dependencies of ROS 2 packages.
RUN apt-get update && apt-get install -y clang-format-3.8 cppcheck git pydocstyle pyflakes python3-coverage python3-flake8 python3-mock python3-nose python3-pep8 uncrustify

# Install build dependencies for turtlebot demo.
RUN if test -n ${INSTALL_TURTLEBOT2_DEMO_DEPS}; apt-get update && apt-get install -y libeigen3-dev libtinyxml-dev
Copy link
Member

Choose a reason for hiding this comment

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

I think you'll need to add this to the top of the dockerfile as with a5ff6c4#diff-a7a7037d3453364950fd80f5f91ed7eeR3

Copy link
Member

Choose a reason for hiding this comment

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

I think you will also want to move this to after this line that prints a different string each day, so that (1) the docker images only diverge once necessary and (2) the docker image gets invalidated daily. this ensures that "fresh" dependencies get installed.

Otherwise, if the text in the dockerfile of what dependencies to install does not change, it will not get re-built even if there are newer versions of dependencies that ought to be installed. For system dependencies it might not matter much but we will probably end up installing ROS packages, which get released regularly. (@dirk-thomas please chime in if this doesn't match what you explained to me the other day!)

This makes three changes requested in review.

From https://github.com/ros2/ci/pull/35/files/fcb54930c990a00d6b29c6ee96f048504eebc62c#r106055638

I started using an inline postfix conditional as in the suggestion but
I'm not sure if we want separate docker image names for the turtlebot
build and the regular linux CI build. It seems like we do, so I made
this a full nested conditional. It could be done with two inline postfix
conditionals testing the same condition or one giant one that
encompasses a good chunk of the command invocation but neither seemed
more readable than just doing this. If we break this out to run on other
linux formats (arm64 or armhf) we could set a buildargs variable earlier
in the template that is either the required build args or an empty
string and add it to the command invocations unconditionally.

From https://github.com/ros2/ci/pull/35/files/fcb54930c990a00d6b29c6ee96f048504eebc62c#r106056720

The second change rework the variable test and set a default value for
the build argument.

The method in the previous commit would have worked, as the test was
checking for a string with nonzero length and would have failed as
desired if no default argument was provided. But it makes complete sense
to match the existing convention of checking for a string matching
`true` rather than just the presence of a variable.

From https://github.com/ros2/ci/pull/35/files/fcb54930c990a00d6b29c6ee96f048504eebc62c#r106058860

Move the dependency installation below the daily invalidation string.
@nuclearsandwich
Copy link
Member Author

You can feel free to test non-dryrun when you want, as this should not affect the existing jobs, just add a new one.

The primary thing holding this back is the lack of a repos file that is turtlebot specific. I suppose I could set it up with the default repos file and make sure that it builds the standard stuff. I'll do that.

@nuclearsandwich
Copy link
Member Author

Testing with the default repos file.

docker build -t ros2_batch_ci linux_docker_resources
@[end if]@
Copy link
Member

Choose a reason for hiding this comment

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

You might want to use to spaces if indentation to make it more readable:

@[  if turtlebot_demo]@
docker build --build-arg INSTALL_TURTLEBOT2_DEMO_DEPS=true -t ros2_batch_ci_turtlebot_demo linux_docker_resources
@[  else]@
docker build -t ros2_batch_ci linux_docker_resources
@[  end if]@

Copy link
Member Author

Choose a reason for hiding this comment

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

I definitely can. Is that kind of indentation a common pattern in empy templates?

Copy link
Member

Choose a reason for hiding this comment

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

We try to use it in our templates (if we don't forget 😉).

Copy link
Member Author

Choose a reason for hiding this comment

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

# Run the turtlebot job on Linux only for now.
os_name = 'linux'
turtlebot_job_data = dict(data)
turtlebot_job_data['os_name'] = os_name
turtlebot_job_data.update(os_configs[os_name])
turtlebot_job_data['turtlebot_demo'] = True
turtlebot_job_data['default_repos_url'] = 'https://raw.githubusercontent.com/ros2/turtlebot2_demo/master/turtlebot2_demo.repos'
# Will use a turtlebot2_demo repos file when one is available.
# turtlebot_job_data['default_repos_url'] = 'https://raw.githubusercontent.com/ros2/turtlebot2_demo/master/turtlebot2_demo.repos'
Copy link
Member

Choose a reason for hiding this comment

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

This is totally fine, but just for your understanding of how the things inter-relate: even with the default_repos_url set to a url that doesn't exist yet, it is just the default. The url can be specified to be something other than the default using the CI_ROS2_REPOS_URL on jenkins when doing "Build with Parameters"

Copy link
Member Author

Choose a reason for hiding this comment

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

it is just the default. The url can be specified to be something other than the default using the CI_ROS2_REPOS_URL on jenkins when doing "Build with Parameters"

Understood, I just didn't want it to be broken by default.

sed -i 's+^FROM.*$+FROM osrf/ubuntu_armhf:xenial+' linux_docker_resources/Dockerfile
docker build --build-arg PLATFORM=arm -t ros2_batch_ci_armhf linux_docker_resources
@[elif os_name == 'linux-aarch64']@
sed -i 's+^FROM.*$+FROM osrf/ubuntu_arm64:xenial+' linux_docker_resources/Dockerfile
docker build --build-arg PLATFORM=arm -t ros2_batch_ci_aarch64 linux_docker_resources
@[elif os_name == 'linux']@
@[if turtlebot_demo]@
docker build --build-arg INSTALL_TURTLEBOT2_DEMO_DEPS=true -t ros2_batch_ci_turtlebot_demo linux_docker_resources
Copy link
Member

Choose a reason for hiding this comment

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

I started using an inline postfix conditional as in the suggestion but
I'm not sure if we want separate docker image names for the turtlebot
build and the regular linux CI build. It seems like we do, so I made
this a full nested conditional.

👍 I didn't think about the name so I agree that this is better as you have it now

Thanks for the other changes in this commit too

@nuclearsandwich
Copy link
Member Author

Sidebar: My editor is crying to me about trailing whitespace on two lines in the job template. Is that whitespace significant or should I just clean that up?

@dhood
Copy link
Member

dhood commented Mar 15, 2017

not significant, feel free to remove it

@nuclearsandwich
Copy link
Member Author

After a couple of errors were ironed out we have a successful build of the default repos file http://ci.ros2.org/job/ci_turtlebot-demo/3/console

@nuclearsandwich
Copy link
Member Author

From https://github.com/ros2/turtlebot2_demo/#install-some-dependencies

The dependencies are ros-kinetic-kobuki-driver ros-kinetic-kobuki-ftdi ros-kinetic-common-msgs ros-kinetic-astra-camera libusb-1.0.0-dev libudev-dev

Does this mean that tinyxml and eigen aren't dependencies for this demo, or that they're pulled in transitively by this dependency set?

@mikaelarguedas
Copy link
Member

These were the dependencies of the previous demo, they will likely be other ones for this demo (I think that at least tinyxml will be necessary to get the robot_state_publisher that @Karsten1987 ported recently to work).

I think it's fine to go with a small set of deps for now and let people add the corresponding dependencies to the dockerfile at the same time they add their packages to the turtlebot repo file.

Regarding this set of dependencies: I didn't chase down the dependency list but very likely these ros packages will pull in most of the dependencies needed for this demo. There is some overlap too: for example ros-kinetic-astra-camera will pull in libusb-1.0.0-dev and libudev-dev automatically

@dhood
Copy link
Member

dhood commented Mar 15, 2017

After a couple of errors were ironed out we have a successful build of the default repos file http://ci.ros2.org/job/ci_turtlebot-demo/3/console

Cool, nice work! Let's revert 12545d4, push the standard ros2.repos file to https://raw.githubusercontent.com/ros2/turtlebot2_demo/master/turtlebot2_demo.repos as a placeholder and merge this. Then we can iterate on the repos file and the dependencies to be installed afterwards.

@nuclearsandwich
Copy link
Member Author

Let's revert 12545d4, push the standard ros2.repos file to https://raw.githubusercontent.com/ros2/turtlebot2_demo/master/turtlebot2_demo.repos as a placeholder and merge this. Then we can iterate on the repos file and the dependencies to be installed afterwards.

9b4ef46 sets the default repos url and dry-ran acceptably.

Do you want to make the repos file push @dhood or shall I PR it?

@dhood
Copy link
Member

dhood commented Mar 15, 2017

9b4ef46 sets the default repos url and dry-ran acceptably.

You will have to re-run create_jenkins_job.py to push the new config to the job. (The default listed in the description of the CI_ROS2_REPOS_URL parameter should update accordingly)

Do you want to make the repos file push @dhood or shall I PR it?

happy for you to thanks and then turtlebot demo developers can iterate on it

@nuclearsandwich
Copy link
Member Author

http://ci.ros2.org/job/ci_turtlebot-demo/7/ passed once I remembered to set the ci scripts branch. Eliminating that problem by merging this PR now.

@nuclearsandwich nuclearsandwich merged this pull request into master Mar 15, 2017
@nuclearsandwich nuclearsandwich removed the in progress Actively being worked on (Kanban column) label Mar 15, 2017
@dhood
Copy link
Member

dhood commented Mar 15, 2017

we usually delete branches once merged if you could please do so when you're done with this branch. Thanks for working on it! 🎉

@dhood
Copy link
Member

dhood commented Mar 15, 2017

Ah also we generally prefer squash-and-merge -- my fault for not mentioning it (it is the default on most repos but not all)

@dirk-thomas
Copy link
Member

Can we please squash the already committed commits on the master branch? Otherwise the verbose history increases future effort in reviewing / tracing past changes significantly.

@nuclearsandwich
Copy link
Member Author

nuclearsandwich commented Mar 15, 2017

Can we please squash the already committed commits on the master branch? Otherwise the verbose history increases future effort in reviewing / tracing past changes significantly.

Before I force push to master, can I confirm that this is what you'd like me to do?

  1. Squash and rebase the commits on this branch onto master
  2. Force push the squashed branch to master.

@dirk-thomas
Copy link
Member

@nuclearsandwich Yes, force push to master to clean up the history. You can also checkout master locally and squash the commits with git rebase -i HEAD~<N>.

@nuclearsandwich
Copy link
Member Author

nuclearsandwich commented Mar 16, 2017

Master has been force pushed. Anyone who pulled the previous history of master after my merge will need to create a new clone or forcibly update their local clone with

git checkout master
git fetch origin master
git reset --hard origin/master

@nuclearsandwich nuclearsandwich deleted the turtlebot-demo branch March 16, 2017 17:26
@mikaelarguedas mikaelarguedas mentioned this pull request Mar 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants