ros-tooling / cross_compile

A tool to build ROS and ROS2 workspaces for various targets

Apache License 2.0

188 stars 60 forks source link

Separate rosdep collection from building #139

Closed emersonknapp closed 4 years ago

emersonknapp commented 4 years ago

Factor out a shared Docker Client for using for different build stages
Rosdeps are collected in a native architecture image and output as a script before creating the sysroot
Use that script in creating the sysroot, so that when workspace sources change the dependencies don't need to be reinstalled

Fixes #106

Note that as of https://github.com/ros-tooling/cross_compile/pull/127 the build step itself became incremental (see https://github.com/ros-tooling/cross_compile/issues/108) - and this PR makes it the case that the whole process performs an incremental build. But, #108 will not be closed by this PR because the completion criteria are not fully met - this currently does incremental by default, whereas that ticket asks for full-rebuild by default.

codecov[bot] commented 4 years ago

Codecov Report

Merging #139 into master will increase coverage by 4.08%. The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #139      +/-   ##
==========================================
+ Coverage   83.56%   87.64%   +4.08%     
==========================================
  Files           5        7       +2     
  Lines         213      251      +38     
==========================================
+ Hits          178      220      +42     
+ Misses         35       31       -4

Flag	Coverage Δ
#unittests	`87.64% <84.05%> (+4.08%)`	:arrow_up:

Impacted Files	Coverage Δ
ros_cross_compile/dependencies.py	`100.00% <0.00%> (ø)`
ros_cross_compile/docker_client.py	`74.19% <0.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 7873318...dfb3cd7. Read the comment docs.

piraka9011 commented 4 years ago

Could this be multiple PRs instead?

emersonknapp commented 4 years ago

The answer to that is usually yes - do you have specific part that you'd really like to see separated?

Edit: I'm going to assume "separate DockerClient refactoring from the new rosdep feature that uses it" - just want to ask for specificity in feedback

emersonknapp commented 4 years ago

Ok I was feeling resistant but I will split it. Looking at this PR again I see:

one bugfix (build_workspace unset variables for sourcing setup.bash)
one factoring-out (DockerClient)
one new feature (separating rosdep gathering)
a bug in the new feature

I'll make this at least 3 PRs

piraka9011 commented 4 years ago

Thanks @emersonknapp! Yes I should have been specific about the docker client :) Taking a look!

thomas-moulard commented 4 years ago

Thanks @emersonknapp ! I forgot to add yesterday, but it'd be good to run all those shell scripts through shellcheck IMHO. Thats what the quoting feedback was about, I just commented on one instance, but I'd rather have a linter list all of those nits for me :)

emersonknapp commented 4 years ago

Closing in favor of more focused split PRs