Fix building on running on Windows Debug. #82
Conversation
In particular, we need to set the python executable properly when running on Windows Debug. While we are in here, we also fix up some dependencies in the package.xml and CMakeLists.txt. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
clalancette
requested review from
jonbinney and
mabelzhang
and removed request for
jonbinney
|
CI:
|
|
The Windows Debug failure is now different. Instead of failing to find the python interpreter, it timed out instead. I've kicked the timeout to 240 seconds, here's another CI run (just Windows Debug for now): |
| @@ -0,0 +1,2 @@ | |||
| [pytest] | |||
| junit_family=xunit2 | |||
There was a problem hiding this comment.
Could you add a comment in this file about why this is needed?
There was a problem hiding this comment.
Sure, done in 606fc20
We can get some deprecation warnings on Windows Debug, so don't enable WERROR. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
clalancette
force-pushed
the
clalancette/fix-windows-debug
branch
from
ef3157c to
78ed5ca
Compare
|
OK, I spent some time with Windows Debug. Basically, we were running too many tests in laser_geometry. I've reduced the set of tests heavily now, so let's try CI on Windows Debug again: |
|
Full CI again:
|
|
I'm not thrilled by the idea of removing tests to make them pass. How long do the tests take with an absurd timeout? It seems odd to me that five extra asserts could be responsible for so much time, but I am not at all familiar with the project (and @Blast545 points out that the time may come from the reduced scan range above). In most cases when tests are timing it is due to a hang or soft lock and not due to an exceptionally long test case so I think we should take a close look to make sure we aren't unintentionally papering over such a discovery. Is it possible with pytest to label tests as performance or time intensive and only have them run if those tests are enabled? I understand the reasoning of not wanting to run these tests in the comprehensive ROS 2 CI for time but it would be nice if we could preserve them for package CI runs. |
They were taking about 500 seconds when running on my Windows VM. That is way too long for the buildfarm, since I don't want to add 10 minutes to the already absurdly long build times.
It's not the asserts that are causing the speedup; I just removed those since they were redundant. What is really causing the speedup is the removal of the options from the lists in the test, like in And the thing is, we really don't need to run all of these tests (at least, not in this package). The Python portion of this package mostly offloads the computation to the |
|
Got it. Thanks for addressing concerns based on naive assumptions. I'm happy if other reviewers are. |
There was a problem hiding this comment.
I'm not one of the maintainers of this package but @Blast545 and I took a look through this PR during our build farmer shift and all of the dependency updates check out, including moving some dependencies from buildtool to test dependencies after moving their find_package() block into the BUILD_TESTING conditional.
I commented previously on the possible negatives of the reduction in test scope but those naive concerns were addressed and I accept the reasoning behind the change.
|
Based on reviews, I'm going to go ahead and merge this one in. This should get us green for Windows Debug. If there are any objections to this PR, please bring them up and I'm happy to do a follow-up to address them. |
clean up eigen export eigen as dependency laser_geometry should export Eigen3 include dirs fix includes for case and type Add visibility header modified from rclcpp Make it compile, remove PointCloud support, and remove boost - Compiles on Windows with VS2015/VS2017 - Compiles on Mac with clang - Compiles on Linux with gcc - Removed PointCloud support as this is deprecated and might not be needed in ROS 2 - Remove boost as per ROS 2 development guidelines Build statically but position independent code - This is necessary to link against shared libraries on Linux Add tests (remove superfluous test cases) and linters - Code now lints with standard ament linters - Added test cases for LaserScan to PointCloud2 - Removed tests that were commented out + tests for LaserScan to PointCloud Uncrustify Use correct time unit Fix cpplint Disable second test for now Test needs a correct lookupTransform It would be best to make that call mockable, but that's not possible with tf2::BufferCore Fix Windows warnings Remove several test cases - On not so fast machines, tests run into timeouts due to exponential explosion: test setup makes for about 7000 test cases - Keep edge cases Remove angle dependency as no longer necessary Add Copyright Fix package.xml Add TODO for PointCloud 1 support Build dynamically using visibility control Make build export symbols Increase tests timeout (needed for Mac) Relicense visibility control file to BSD Already relicensed in URDF repo. Create LICENSE (ros-perception#33) (ros-perception#34) fix eigen dependency name (ros-perception#36) fixup package.xml changelogs 2.0.0 Use eigen3_cmake_module Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> 2.1.0 Signed-off-by: Michael Carroll <michael@openrobotics.org> Drop CMake extras redundant with eigen3_cmake_module. (ros-perception#50) Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com> code style only: wrap after open parenthesis if not in one line (ros-perception#52) Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com> use target_include_directories Signed-off-by: Karsten Knese <karsten@openrobotics.org> use ament_export_targets() Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com> Changelog. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> 2.2.0 increase test timeout Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com> update maintainers Signed-off-by: Mabel Zhang <mabel@openrobotics.org> Use rclcpp::Duration::from_seconds (ros-perception#72) Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com> Changelog. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> 2.2.1 Export sensor_msgs, tf2, and rclcpp as dependencies Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com> changelog Signed-off-by: Mabel Zhang <mabel@openrobotics.org> 2.2.2 Update python code and tests for ros2 (ros-perception#80) * Enable projection nose test using ament * Update python package and tests for ros2 * Remove unneeded python setup file * Use pytest instead of nose Nose was outputting xml that xUnit (jenkins plugin) couldn't read. * Fix pytest warnings Fix building on running on Windows Debug. (ros-perception#82) * Fix building on running on Windows Debug. In particular, we need to set the python executable properly when running on Windows Debug. While we are in here, we also fix up some dependencies in the package.xml and CMakeLists.txt. We also have to remove WERROR ON, due to some Python warnings that are outside of our control. Finally, we heavily reduce the number of tests being run here so that the tests complete in a reasonable amount of time, even on (slow) Windows debug. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> 2.3.0 Fix Duration casting issue leading to no undistortion Signed-off-by: Marco Lampacrescia <Marco.Lampacrescia@de.bosch.com> Explicit cast to double to prevent loss of precision Signed-off-by: Marco Lampacrescia <Marco.Lampacrescia@de.bosch.com> Install headers to include/${PROJECT_NAME} (ros-perception#86) Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> Changelog. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> 2.4.0 Mirror rolling to ros2 Update Maintainers (ros-perception#88) * Add Dharini Dutia to CODEOWNERs file Signed-off-by: Audrow Nash <audrow@openrobotics.org> Update laser_geometry to C++17. (ros-perception#90) The main reason to do this is so that we can compile laser_geometry with the clang static analyzer. As of clang++-14 (what is in Ubuntu 22.04), the default still seems to be C++14, so we need to specify C++17 so that new things in the rclcpp headers work properly. Further, due to reasons I don't fully understand, I needed to set CMAKE_CXX_STANDARD_REQUIRED in order for clang to really use that version. So set this as well. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> Changelog. Signed-off-by: Michael Carroll <michael@openrobotics.org> 2.5.0 Changelog Signed-off-by: Chris Lalancette <clalancette@gmail.com> 2.6.0 Switch to target_link_libraries. (ros-perception#92) This allows us to hide more of the libraries from downstream consumers. While we are in here, do slight cleanups so it is more clear which libraries are depended on. Signed-off-by: Chris Lalancette <clalancette@gmail.com> Changelog. Signed-off-by: Chris Lalancette <clalancette@gmail.com> 2.7.0 Changelog. Signed-off-by: Marco A. Gutierrez <marcogg@marcogg.com> 2.8.0 Added common linters (ros-perception#96) Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com> Changelog. Signed-off-by: Chris Lalancette <clalancette@gmail.com> 2.8.1 Stop using python_cmake_module. (ros-perception#93) * Stop using python_cmake_module. We really don't need it anymore, and can just use the builtin find_package(Python3). * Add in missing ament_cmake_python dependency. Signed-off-by: Chris Lalancette <clalancette@gmail.com>
In particular, we need to set the python executable properly
when running on Windows Debug. While we are in here, we also
fix up some dependencies in the package.xml and CMakeLists.txt.
Signed-off-by: Chris Lalancette clalancette@openrobotics.org
This should fix up the Windows Debug failures we are seeing after #80 was merged: https://ci.ros2.org/view/nightly/job/nightly_win_deb/2099/testReport/junit/(root)/laser_geometry/___/
@jonbinney FYI.