Conditionally exclude test dependencies #649

cottsay · 2021-08-24T23:37:56Z

In the debian generator, we can decorate the BuildDepends to omit them if the nocheck flag is set.

For RPMs, there isn't a standard mechanism for skipping tests but we actually created a build flag for skipping tests, so the only work here was to wire that custom flag into the dependency enumeration.

I still need to do some testing on the debian side. In particular, I'm not sure if nocheck automatically sets ENABLE_TESTING=OFF in the cmake builds. Either way, I think we'll need to update it to set CATKIN_ENABLE_TESTING appropriately in the catkin template.

In the debian generator, we can decorate the BuildDepends to omit them if the `nocheck` flag is set. For RPMs, there isn't a standard mechanism for skipping tests but we actually created a build flag for skipping tests, so the only work here was to wire that custom flag into the dependency enumeration.

nuclearsandwich

What's here looks good so far. I can schedule some exploratory testing of the debian behavior with nocheck and propose changes to the debian templates if needed.

j-rivero

In the debian generator, we can decorate the BuildDepends to omit them if the nocheck flag is set.

The <!nocheck> in Debian is called a Build-Profile and it's being used in the control file to specify which packages are excluded for testing when the user ask for this kind of "profile". In order to use that profile, we need to set it in DEB_BUILD_OPTIONS and that variable need to arrive to dpkg-buildpackage. At least, two ways for that I can think of:

Set DEB_BUILD_OPTIONS=nocheck in the rules file via empy templates
Pass DEB_BUILD_OPTIONS=nocheck when invoking the build command (not sure if this repo is using debbuild or dpkg-buildpackage calls directly).

I still need to do some testing on the debian side. In particular, I'm not sure if nocheck automatically sets ENABLE_TESTING=OFF in the cmake builds.

I don't think that Debian handles the ENABLE_TESTING variable in anyway and did not find any reference in code to nocheck build profile particularly. I found some packages implementing that option, is not particularly difficult, just check for nocheck in DEB_BUILD_OPTIONS and do the magic in the configure step. See https://sources.debian.org/src/foonathan-memory/0.7.1-4/debian/rules/?hl=43#L43

bloom/generators/debian/generator.py

cottsay · 2021-10-25T18:01:32Z

Pass DEB_BUILD_OPTIONS=nocheck when invoking the build command (not sure if this repo is using debbuild or dpkg-buildpackage calls directly).

This was attempted in ros-infrastructure/ros_buildfarm#903 already.

...just check for nocheck in DEB_BUILD_OPTIONS and do the magic in the configure step.

Ah, thanks for the confirmation. I'll add that snippet to this PR then.

cottsay · 2021-10-25T21:13:10Z

Finally got around to doing some testing on this, and it looks like ros_buildfarm will need to be updated to handle the annotated debian deps.

j-rivero · 2021-10-25T21:30:50Z

Finally got around to doing some testing on this, and it looks like ros_buildfarm will need to be updated to handle the annotated debian deps.

If it helps you, the build profile document provides information on which pieces in the Debian software ecosystem needs to be in place to handle this https://wiki.debian.org/BuildProfileSpec#Document_status

Of course that does not mean that our code can work with them but at least we need these set of base packages available to make it work.

cottsay · 2021-10-25T23:55:14Z

Alright I got this working locally, but I need to resolve these issues:

As was already pointed out, I need to update this PR to selectively disable BUILD_TESTING based on nocheck. Yet to be determined is whether to use DEB_BUILD_OPTIONS or DEB_BUILD_PROFILES.
We're manually parsing Build-Depends out of the dsc in ros_buildfarm, and that code needs to be updated to support the profile exclusions. I'll open a PR.
It turns out that there are some problems in the build chain regarding nocheck. dpkg-checkbuilddeps is ignoring DEB_BUILD_OPTIONS and instead looks for DEB_BUILD_PROFILES=nocheck, but dh_auto_test is looking for DEB_BUILD_OPTIONS and ignores DEB_BUILD_PROFILES. I'm inclined to add the -DBUILD_TESTING=OFF flag to rules.em based on DEB_BUILD_OPTIONS purely based on proximity to dh_auto_test, but no matter what, it seems that I'll need to update ros_buildfarm to set them both to please dpkg-checkbuilddeps.

I'm wading through mailing lists trying to figure out if this is expected behavior or if I'm missing something.

j-rivero · 2021-10-26T18:26:40Z

1. As was already pointed out, I need to update this PR to selectively disable `BUILD_TESTING` based on `nocheck`. Yet to be determined is whether to use `DEB_BUILD_OPTIONS` or `DEB_BUILD_PROFILES`.

Good point. The document seems to recommend do it on both:

No test suite should be run, and build dependencies used only for that purpose should be ignored. Builds that set this profile must also add nocheck to DEB_BUILD_OPTIONS

3\. It turns out that there are some problems in the build chain regarding `nocheck`. `dpkg-checkbuilddeps` is ignoring `DEB_BUILD_OPTIONS` and instead looks for `DEB_BUILD_PROFILES=nocheck`, but `dh_auto_test` is looking for `DEB_BUILD_OPTIONS` and ignores `DEB_BUILD_PROFILES`. I'm inclined to add the `-DBUILD_TESTING=OFF` flag to `rules.em` based on `DEB_BUILD_OPTIONS` purely based on proximity to `dh_auto_test`, but no matter what, it seems that I'll need to update `ros_buildfarm` to set them both to please `dpkg-checkbuilddeps`.

We probably need to set both of them. To handle -DBUILD_TESTING=OFF I see packages in Debian doing both :) so +1 to use DEB_BUILD_OPTIONS.

cottsay · 2021-10-26T18:29:38Z

Okay, my latest commit in this PR when combined with ros-infrastructure/ros_buildfarm#918 and ros-infrastructure/ros_buildfarm#919 is working as expected. I manually tested ament_cmake and catkin packages from ROS 2 and ROS 1, and I didn't see anything strange happening.

I'm still a little bit unsure about setting the nocheck profile in multiple places, but it appears to work.

j-rivero

Good to go, with green CI

nuclearsandwich

I think this change is significant enough that we should bump to the next minor (as bloom is <1.0) and make an announcement of this change (and the related changes in ros_buildfarm) on ROS Discourse.

The changes suggested are just that and completely discretionary.

bloom/generators/debian/generator.py

nuclearsandwich · 2021-10-27T18:08:24Z

bloom/generators/rpm/templates/ament_python/template.spec.em

+@[if TestDepends]@\n%if 0%{?with_tests}
+@[for p in TestDepends]BuildRequires:  @p@\n@[end for]@
+%endif@\n@[end if]@


This is just preference and so take it or leave it but I find the embedded newlines less readable than plaintext newlines. This should generate the same result.

Suggested change

@[if TestDepends]@\n%if 0%{?with_tests}

@[for p in TestDepends]BuildRequires: @p@\n@[end for]@

%endif@\n@[end if]@

@[if TestDepends]@

%if 0%{?with_tests}

@[for p in TestDepends]@

BuildRequires: @p

@[end for]@

%endif

@[end if]@

nuclearsandwich · 2021-10-27T18:10:31Z

bloom/generators/rpm/templates/ament_cmake/template.spec.em

+@[if TestDepends]@\n%if 0%{?with_tests}
+@[for p in TestDepends]BuildRequires:  @p@\n@[end for]@
+%endif@\n@[end if]@


This is just preference and so take it or leave it but I find the embedded newlines less readable than plaintext newlines. This should generate the same result.

Suggested change

@[if TestDepends]@\n%if 0%{?with_tests}

@[for p in TestDepends]BuildRequires: @p@\n@[end for]@

%endif@\n@[end if]@

@[if TestDepends]@

%if 0%{?with_tests}

@[for p in TestDepends]@

BuildRequires: @p@

@[end for]@

%endif

@[end if]@

nuclearsandwich · 2021-10-27T18:11:54Z

bloom/generators/rpm/templates/catkin/template.spec.em

+@[if TestDepends]@\n%if 0%{?with_tests}
+@[for p in TestDepends]BuildRequires:  @p@\n@[end for]@
+%endif@\n@[end if]@


This is just preference and so take it or leave it but I find the embedded newlines less readable than plaintext newlines. This should generate the same result.

Suggested change

@[if TestDepends]@\n%if 0%{?with_tests}

@[for p in TestDepends]BuildRequires: @p@\n@[end for]@

%endif@\n@[end if]@

@[if TestDepends]@

%if 0%{?with_tests}

@[for p in TestDepends]@

BuildRequires: @p

@[end for]@

%endif

@[end if]@

nuclearsandwich · 2021-10-27T18:12:50Z

bloom/generators/rpm/templates/cmake/template.spec.em

+@[if TestDepends]@\n%if 0%{?with_tests}
+@[for p in TestDepends]BuildRequires:  @p@\n@[end for]@
+%endif@\n@[end if]@


This is just preference and so take it or leave it but I find the embedded newlines less readable than plaintext newlines. This should generate the same result.

Suggested change

@[if TestDepends]@\n%if 0%{?with_tests}

@[for p in TestDepends]BuildRequires: @p@\n@[end for]@

%endif@\n@[end if]@

@[if TestDepends]@

%if 0%{?with_tests}

@[for p in TestDepends]@

BuildRequires: @p

@[end for]@

%endif

@[end if]@

this is implicitly pulled-in through the test depends on rosunit, but fails with ros-infrastructure/bloom#649 which disables test dependencies.

This package is not very critical and it is unclear what it should do at all when testing is disabled. But as the CMakeLists.txt requires these packages during either build, they are NOT just test dependencies. Required to make use of added functions to exclude test depends in builds ros-infrastructure/bloom#649

this is implicitly pulled-in through the test depends on rosunit, but fails with ros-infrastructure/bloom#649 which disables test dependencies.

cottsay added the enhancement label Aug 24, 2021

cottsay self-assigned this Aug 24, 2021

Reorder RPM dep blocks to mirror bcond_without order

2392510

nuclearsandwich reviewed Oct 12, 2021

View reviewed changes

cottsay marked this pull request as ready for review October 18, 2021 15:31

j-rivero requested changes Oct 25, 2021

View reviewed changes

bloom/generators/debian/generator.py Show resolved Hide resolved

Still want to resolve the test deps, of course

31f1a81

Disable tests in CMake debs if nocheck

174a314

Merge branch 'master' into cottsay/no-check-deps

9741e22

j-rivero approved these changes Oct 27, 2021

View reviewed changes

nuclearsandwich approved these changes Oct 27, 2021

View reviewed changes

Add a comment about <!nocheck>

345464f

cottsay merged commit 53beb52 into master Oct 28, 2021

cottsay deleted the cottsay/no-check-deps branch October 28, 2021 23:34

tonybaltovski mentioned this pull request May 3, 2022

New release? ros-infrastructure/ros_buildfarm#963

Open

nuclearsandwich mentioned this pull request May 26, 2023

add meson support #691

Open

v4hn added a commit to ros-o/ros_comm that referenced this pull request Oct 11, 2024

add missing python dependency

e57d8a3

this is implicitly pulled-in through the test depends on rosunit, but fails with ros-infrastructure/bloom#649 which disables test dependencies.

v4hn added a commit to ros-o/ros_comm that referenced this pull request Oct 11, 2024

add missing python dependency

164a2ac

this is implicitly pulled-in through the test depends on rosunit, but fails with ros-infrastructure/bloom#649 which disables test dependencies.

v4hn added a commit to ros-o/ros_comm that referenced this pull request Oct 14, 2024

add missing python dependency

ff1baa3

this is implicitly pulled-in through the test depends on rosunit, but fails with ros-infrastructure/bloom#649 which disables test dependencies.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Conditionally exclude test dependencies #649

Conditionally exclude test dependencies #649

cottsay commented Aug 24, 2021

nuclearsandwich left a comment

j-rivero left a comment

cottsay commented Oct 25, 2021

cottsay commented Oct 25, 2021

j-rivero commented Oct 25, 2021 •

edited

Loading

cottsay commented Oct 25, 2021

j-rivero commented Oct 26, 2021

cottsay commented Oct 26, 2021

j-rivero left a comment

nuclearsandwich left a comment

nuclearsandwich Oct 27, 2021

nuclearsandwich Oct 27, 2021

nuclearsandwich Oct 27, 2021

nuclearsandwich Oct 27, 2021

Conditionally exclude test dependencies #649

Conditionally exclude test dependencies #649

Conversation

cottsay commented Aug 24, 2021

nuclearsandwich left a comment

Choose a reason for hiding this comment

j-rivero left a comment

Choose a reason for hiding this comment

cottsay commented Oct 25, 2021

cottsay commented Oct 25, 2021

j-rivero commented Oct 25, 2021 • edited Loading

cottsay commented Oct 25, 2021

j-rivero commented Oct 26, 2021

cottsay commented Oct 26, 2021

j-rivero left a comment

Choose a reason for hiding this comment

nuclearsandwich left a comment

Choose a reason for hiding this comment

nuclearsandwich Oct 27, 2021

Choose a reason for hiding this comment

nuclearsandwich Oct 27, 2021

Choose a reason for hiding this comment

nuclearsandwich Oct 27, 2021

Choose a reason for hiding this comment

nuclearsandwich Oct 27, 2021

Choose a reason for hiding this comment

j-rivero commented Oct 25, 2021 •

edited

Loading