Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Implement TEST=debians #719

Merged

Conversation

mathias-luedtke
Copy link
Member

No description provided.

cd "$pkg_path"
ici_quiet_echo bloom-generate rosdebian --ros-distro="$ROS_DISTRO" --debian-inc="ici~"
ici_quiet_echo dpkg-buildpackage -b -uc -us
mv ../*.{buildinfo,changes,deb} .

Choose a reason for hiding this comment

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

superflous

Copy link
Member Author

Choose a reason for hiding this comment

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

Why?

Choose a reason for hiding this comment

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

please give reasons why you need this (instead of just using .. in the path for dpkg/apt below.

Copy link
Member Author

Choose a reason for hiding this comment

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

To only install a single *.deb file, and not all of them every time.

Choose a reason for hiding this comment

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

hmkay, maybe add a comment then.

ici_quiet_echo bloom-generate rosdebian --ros-distro="$ROS_DISTRO" --debian-inc="ici~"
ici_quiet_echo dpkg-buildpackage -b -uc -us
mv ../*.{buildinfo,changes,deb} .
ici_quiet_echo ici_asroot dpkg -i ./*.deb

Choose a reason for hiding this comment

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

Suggested change
ici_quiet_echo ici_asroot dpkg -i ./*.deb
ici_quiet_echo ici_asroot apt-get install ./*.deb

(will resolve dependencies as well)

Copy link
Member Author

Choose a reason for hiding this comment

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

(will resolve dependencies as well)

It should must resolve dependencies!
All dependencies should already be installed.

Choose a reason for hiding this comment

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

you should only install build dependencies to build the package, but installing the build package could bring additional run dependencies.

Copy link
Member Author

Choose a reason for hiding this comment

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

but installing the build package could bring additional run dependencies.

I just wanted to avoid pulling in the already built packages from the buildfarm.

Choose a reason for hiding this comment

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

but then dpkg could fail and thus your CI job (I guess).

Copy link
Member Author

Choose a reason for hiding this comment

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

but then dpkg could fail and thus your CI job (I guess).

Yes, that's what I wanted to test for..

Choose a reason for hiding this comment

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

Just for completeness, I think it could happen that colcons topological sort builds a package A before B but A needs B to get installed. So it could be that a package is not installable after it was build. You could add an install test with all packages at the end, maybe.

ici_run "install_${name}_dependencies" ici_quiet ici_install_dependencies "$UNDERLAY" "$ROSDEP_SKIP_KEYS" "$sourcespace"

while read -r -a pkg; do
ici_run "${pkg[0]}_debian" build_and_install_debian "$current/${pkg[1]}"

Choose a reason for hiding this comment

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

this would still all be run in the same root filesystem, right?
The point of sbuild is to have a minimal chroot (container) (only essential packages installed) to find missing dependencies. With your setup if you have a repo with multiple ROS packages and package A ab B have a dependency X but only A declares it, packages B would still build if the dependency was installed for A before. That would not happen with sbuild nor work on the ROS build farm (and was the point in #697).

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 see. Not sure how to solve this. sbuild is just too complicated to set-up (incl. some manual steps).
The ROS pre-release test builds each package in a clean environment already, so I wonder why it did not fail in the first place.

Choose a reason for hiding this comment

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

which pre-release tests do you mean?
http://prerelease.ros.org/ does not, afaik.

Choose a reason for hiding this comment

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

Regarding sbuild being too complicated, you could spin up a new docker container for every package

Copy link
Member Author

Choose a reason for hiding this comment

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

which pre-release tests do you mean?

http://prerelease.ros.org/ does not, afaik.

This one, but with without the need to register it in rosdistro.
I thought that it creates a new container (actually, new containers) for each package, but after digging into the log files this might not be the case :/

Regarding sbuild being too complicated, you could spin up a new docker container for every package

Yes, I will try this next. Together with a shared, local repository.

Copy link
Member Author

Choose a reason for hiding this comment

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

The latest version does this, at least locally.
I will provide a test case as well later.

industrial_ci/src/tests/debians.sh Outdated Show resolved Hide resolved
@mathias-luedtke mathias-luedtke force-pushed the feature/test-debians branch 4 times, most recently from 9c0e6f7 to 5e8f106 Compare July 22, 2021 21:14
@mathias-luedtke mathias-luedtke force-pushed the feature/test-debians branch 8 times, most recently from 00fda10 to 0a581b3 Compare July 22, 2021 23:02
Copy link

@jspricke jspricke 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 looks good (but didn't look to deep), Thanks for working on this!

Maybe a test with multiple packages would be a good addition (did you try it with Moveit already?)

)

function run_debians() {
# shellcheck source=industrial_ci/src//isolation/docker.sh

Choose a reason for hiding this comment

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

nit: '//'

export BUILDER=colcon
ici_source_builder
ici_run "${BUILDER}_setup" ici_quiet builder_setup
ici_run "setup_bloom" ici_quiet ici_install_pkgs_for_command bloom-generate python3-bloom debhelper

Choose a reason for hiding this comment

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

why do you need debhelper here?

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 not needed anymore, I will remove it!

@mathias-luedtke
Copy link
Member Author

Maybe a test with multiple packages would be a good addition

The testcase already builds two packages

did you try it with Moveit already?

I am on it: https://github.com/ipa-mdl/industrial_ci/actions/runs/1059400937

@jspricke
Copy link

The testcase already builds two packages

Ah, missed the industrial-ci package. Maybe add a dependency between them?

I am on it: https://github.com/ipa-mdl/industrial_ci/actions/runs/1059400937

Awesome, nice result :).

@mathias-luedtke
Copy link
Member Author

Awesome, nice result :).

Actually, it failed for a different reason (I believe because of repo mixups)
However, https://github.com/ipa-mdl/industrial_ci/actions/runs/1059855175 fails with:

dpkg-shlibdeps: error: no dependency information found for /opt/ros/galactic/lib/libmoveit_ros_occupancy_map_monitor.so. (used by debian/ros-galactic-moveit-ros-planning/opt/ros/galactic/lib/libmoveit_planning_scene_monitor.so.)
Hint: check if the library actually comes from a package.

Just like the build farm.

Maybe add a dependency between them?

The two packages must be used separately.
But we should add a proper test case. Either an artificial one or a real pepository.

@mathias-luedtke mathias-luedtke force-pushed the feature/test-debians branch 10 times, most recently from 4b4edf0 to cc0be4a Compare July 29, 2021 20:51
@mathias-luedtke mathias-luedtke force-pushed the feature/test-debians branch 2 times, most recently from 3ba1f2b to d8481fb Compare August 2, 2021 08:46
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants