Skip to content
This repository has been archived by the owner on Dec 14, 2022. It is now read-only.

First pass at specifying a custom docker image #330

Closed
wants to merge 1 commit into from

Conversation

kjeremy
Copy link
Contributor

@kjeremy kjeremy commented Jun 29, 2021

@kjeremy
Copy link
Contributor Author

kjeremy commented Jun 29, 2021

Can I get some guidance on writing a test for this?

@codecov
Copy link

codecov bot commented Jun 29, 2021

Codecov Report

Merging #330 (9875324) into master (6414428) will decrease coverage by 0.51%.
The diff coverage is 75.00%.

❗ Current head 9875324 differs from pull request most recent head 28b71be. Consider uploading reports for the commit 28b71be to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #330      +/-   ##
==========================================
- Coverage   93.23%   92.72%   -0.52%     
==========================================
  Files          11       11              
  Lines         414      426      +12     
==========================================
+ Hits          386      395       +9     
- Misses         28       31       +3     
Flag Coverage Δ
unittests 92.72% <75.00%> (-0.52%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
ros_cross_compile/ros_cross_compile.py 83.14% <57.14%> (-2.22%) ⬇️
ros_cross_compile/platform.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6414428...28b71be. Read the comment docs.

@emersonknapp
Copy link
Contributor

This looks pretty good! note - we'll want an addition to the README as well once this is finalized.

Can I get some guidance on writing a test for this?

Some simple unit testing around Platform would be good. There isn't (currently) a Python-based test that runs the full pipeline to try out the CLI entrypoint in that way - but the Github Action workflows call run_e2e_test.sh, you could add another test condition there to try out this with a base image. I might try using rostooling/setup-ros-docker:ubuntu-focal-ros-foxy-ros-base-latest as the override sysroot, for something that only needs dependencies from the ros-base set.

@kjeremy kjeremy force-pushed the use-build-image branch 2 times, most recently from 1d8990e to 9875324 Compare June 29, 2021 20:56
@kjeremy
Copy link
Contributor Author

kjeremy commented Jun 29, 2021

@emersonknapp

In my testing this doesn't seem to trigger a build. Do I need to somehow copy the build_workspace.sh file into the container and execute it?

@emersonknapp
Copy link
Contributor

In my testing this doesn't seem to trigger a build. Do I need to somehow copy the build_workspace.sh file into the container and execute it?

Oh - yes, I made a mistake suggesting that image as the sysroot. Given the way the build currently works, you'll probably need to match the setup from this line on https://github.com/ros-tooling/cross_compile/blob/master/ros_cross_compile/docker/sysroot.Dockerfile#L89

Thinking about it further, this feature is probably best suited (in the first version) to just reusing an image from a prior cross-compile build. Does that sound right to you? If so, then we could somehow specify or get the name of the sysroot image from a previous test build, and run using that.

@kjeremy
Copy link
Contributor Author

kjeremy commented Jun 30, 2021 via email

@emersonknapp
Copy link
Contributor

Ultimately I would like to be able to save that
earlier generated image off (push it to my gitlab registry) and refer to it
in the next invocation to actually use it.

Makes sense! That should work with this approach - the main point is we need to use a sysroot image that was generated by the tool, rather than some other bare image.

@kjeremy
Copy link
Contributor Author

kjeremy commented Jul 1, 2021 via email

@emersonknapp
Copy link
Contributor

If so could we use one of the existing customization script points to push
the image to a registry or would the program need to be modified to do that
as well?

There's not a good way to hook this in as-is, since the customization scripts run inside a container, rather than on the host.

High level, what could be done is

  • Run with only the sysroot step (with an added custom tag option?)
  • Upload that tag to a registry
  • Run the tool again, starting at build step

@MichaelOrlov
Copy link
Member

  • Closing this PR as stale and wouldn't be merged since we are going to archive target repo.
    Feel free to reopen if you think will be able to make it ready to be merged in one week time frame.

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