Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

feat(clang-tidy, colcon-build): make CMAKE_BUILD_TYPE optional #185

Merged
merged 3 commits into from
Sep 28, 2022

Conversation

kenji-miyake
Copy link
Contributor

@kenji-miyake kenji-miyake commented Sep 27, 2022

Description

Release can overlook some mistakes.

Related: autowarefoundation/autoware.universe#1959

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

  • There are no open discussions or they are tracked via tickets.

After all checkboxes are checked, anyone who has write access can merge the PR.

@kenji-miyake
Copy link
Contributor Author

From this comment, I'll use Debug.
autowarefoundation/autoware.universe#1959 (comment)

@kenji-miyake kenji-miyake changed the title fix(clang-tidy, colcon-build): use RelWithDebInfo for CMAKE_BUILD_TYPE fix(clang-tidy, colcon-build): use Debug for CMAKE_BUILD_TYPE Sep 27, 2022
Signed-off-by: Kenji Miyake <[email protected]>
@ambroise-arm
Copy link
Contributor

What I had in mind was adding a step to also compile and run tests with Debug, not replacing Release. I believe continuing to test Release is important because it reflects the state that the end application will (or at least should) be using.
Doing both compilations and tests all the time would probably be wasteful in time and resources. So maybe only on the nightly trigger?

But I was checking locally the result of colcon test on the whole autoware repository and I get a lot of packages failing:

14 packages had test failures: autoware_auto_control_msgs autoware_auto_geometry_msgs autoware_auto_mapping_msgs autoware_auto_perception_msgs autoware_auto_planning_msgs autoware_auto_system_msgs autoware_auto_vehicle_msgs autoware_control_msgs autoware_localization_msgs autoware_sensing_msgs behavior_velocity_planner freespace_planning_algorithms tamagawa_imu_driver velodyne_msgs

So maybe it's not such a good idea to add Debug right now. I think it would be good to look into each failure and fixing those first before adding it. For example, behavior_velocity_planner failed with:

2: occlusion_spot-test: /usr/include/eigen3/Eigen/src/Core/DenseCoeffsBase.h:117: Eigen::DenseCoeffsBase<Derived, 0>::CoeffReturnType Eigen::DenseCoeffsBase<Derived, 0>::operator()(Eigen::Index, Eigen::Index) const [with Derived = Eigen::Matrix<float, -1, -1>; Eigen::DenseCoeffsBase<Derived, 0>::CoeffReturnType = const float&; Eigen::Index = long int]: Assertion `row >= 0 && row < rows() && col >= 0 && col < cols()' failed.

which doesn't look great.

@kenji-miyake
Copy link
Contributor Author

@ambroise-arm Thank you for your comment. Okay, then I'll make it an option.

Signed-off-by: Kenji Miyake <[email protected]>
@kenji-miyake
Copy link
Contributor Author

@ambroise-arm How about this?

@kenji-miyake kenji-miyake changed the title fix(clang-tidy, colcon-build): use Debug for CMAKE_BUILD_TYPE feat(clang-tidy, colcon-build): make CMAKE_BUILD_TYPE optional Sep 27, 2022
@ambroise-arm
Copy link
Contributor

@kenji-miyake Yep, looks good!

@kenji-miyake
Copy link
Contributor Author

Thanks. I'll test it in autoware.universe.

@kenji-miyake
Copy link
Contributor Author

Seems it's working.
autowarefoundation/autoware.universe#1965

@kenji-miyake kenji-miyake merged commit ebc6112 into main Sep 28, 2022
@kenji-miyake kenji-miyake deleted the change-cmake-build-type branch September 28, 2022 06:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants