Remove unused dependency to save build time#5391
Remove unused dependency to save build time#5391mini-1235 wants to merge 1 commit intoros-navigation:mainfrom
Conversation
|
@mini-1235, your PR has failed to build. Please check CI outputs and resolve issues. |
|
Do you notice a difference from these changes? |
|
@mini-1235, your PR has failed to build. Please check CI outputs and resolve issues. |
1 similar comment
|
@mini-1235, your PR has failed to build. Please check CI outputs and resolve issues. |
Yes, on my machine (XPS 15 9020), it saves around 30 seconds I will do a second check tonight, there are probably more that I can remove |
|
@mini-1235, your PR has failed to build. Please check CI outputs and resolve issues. |
|
@mini-1235, your PR has failed to build. Please check CI outputs and resolve issues. |
2 similar comments
|
@mini-1235, your PR has failed to build. Please check CI outputs and resolve issues. |
|
@mini-1235, your PR has failed to build. Please check CI outputs and resolve issues. |
|
This is kind of odd that its failing to build in time. In this file, there are 3x instances of v36. Bump to v37 and see if that helps. It may not, but busting the cache is a first step before digging deeper. |
|
@mini-1235, your PR has failed to build. Please check CI outputs and resolve issues. |
|
@mini-1235 rebase once #5409 is merged (in the next hour or so) and this should turn over and I can merge it :-) CI for some reason has broken past the 1 hour mark per job and I'm making a semi-temporary workaround to get things going again. Make sure to also handle the merge conflict. |
Ahh ok, just saw this, I will rebased once #5414 is done |
|
@mini-1235 that PR is blocked from the other change for the nav2_utils presently. This puts us at a few blocked PRs stacked up :( |
|
This pull request is in conflict. Could you fix it @mini-1235? |
|
@mini-1235 have you considered searching also for |
Not really, I guess that will be a lot of work, but I will check it in a few days |
ec8b2ad to
0cd6e0d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests.
... and 10 files with indirect coverage changes 🚀 New features to boost your workflow:
|
I ran ClangBuildAnalyzer on nav2_costmap_2d and nav2_behavior_tree—these two packages alone take about one third of the total build time on my machine. As expected, rclcpp/rclcpp.hpp is a very expensive header: But since we are not following the "include what you use" principle in this repo, refactoring such large packages to replace broad includes would require extensive changes. For this PR, I will focus on improving the CMakeLists and only optimize header includes in smaller packages. Sorry about that :( |
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
0cd6e0d to
50dc872
Compare
|
To be honest, I am not really satisfied with this PR, so I guess I will close this temporarily :( |
|
Ok! Sorry to hear that - I’m ok with another philosophy for includes. Maybe this would be good to file a ticket and provide this different philosophy, rationale, and what we should do differently (maybe a couple of examples?). If it speeds up build times and we can document it in our contribution guide, I’m definitely open to improvements 🙂 |
Basic Info
Description of contribution in a few bullet points
Recently, I noticed that it takes a long time to build nav2 from source on my machine, I also found that there are some unused packages in CMakelist and package.xml, this PR removed those are unused, hopefully this saves a little bit build time
Description of documentation updates required from your changes
Description of how this change was tested
Future work that may be required in bullet points
For Maintainers:
backport-*.