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

Colcon plugin: use release build by default #4502

Merged

Conversation

MirkoFerrati
Copy link
Contributor


Normally Colcon does not set any build flags and leaves the choice to the user, meaning the default value is empty and uses the default cmake configuration for debug symbols and optimization.
Inside snaps the executables are not supposed to be debugged and should be as optimized as possible so we want the default to be "Release" explicitly, unless the user overrides it intentionally in their snapcraft.yaml
This PR has been tested by creating snaps of the simple ros2-talker-listener snap with different debug levels, either default or user-customized.
The results for the talker executable show a clear reduction in size and in the number of symbols.

snapcraft/parts/plugins/colcon_plugin.py Outdated Show resolved Hide resolved
snapcraft/parts/plugins/colcon_plugin.py Outdated Show resolved Hide resolved
tests/legacy/unit/plugins/v2/test_colcon.py Outdated Show resolved Hide resolved
Copy link
Contributor Author

@MirkoFerrati MirkoFerrati left a comment

Choose a reason for hiding this comment

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

I will push changes right now

snapcraft/parts/plugins/colcon_plugin.py Outdated Show resolved Hide resolved
tests/legacy/unit/plugins/v2/test_colcon.py Outdated Show resolved Hide resolved
snapcraft/parts/plugins/colcon_plugin.py Outdated Show resolved Hide resolved
@MirkoFerrati MirkoFerrati force-pushed the feature/ros_colcon_release_build branch from 950f960 to b522214 Compare January 4, 2024 16:50
@codecov-commenter
Copy link

codecov-commenter commented Jan 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0b6fecb) 89.20% compared to head (c20d3fb) 89.20%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4502   +/-   ##
=======================================
  Coverage   89.20%   89.20%           
=======================================
  Files         321      321           
  Lines       21755    21759    +4     
=======================================
+ Hits        19407    19411    +4     
  Misses       2348     2348           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tests/legacy/unit/plugins/v2/test_colcon.py Outdated Show resolved Hide resolved
tests/unit/parts/plugins/test_colcon.py Outdated Show resolved Hide resolved
@MirkoFerrati MirkoFerrati force-pushed the feature/ros_colcon_release_build branch from b522214 to 7a266a5 Compare January 5, 2024 11:32
@artivis
Copy link
Contributor

artivis commented Jan 5, 2024

@mr-cal Can we get an approval for the CI?

@mr-cal mr-cal requested a review from cmatsuoka January 5, 2024 17:00
Copy link
Contributor

@cmatsuoka cmatsuoka left a comment

Choose a reason for hiding this comment

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

Thanks!

@cmatsuoka
Copy link
Contributor

Should this be also backported to 7.x?

@MirkoFerrati
Copy link
Contributor Author

@cmatsuoka no need to backport

@mr-cal mr-cal merged commit b6c5a60 into canonical:main Jan 8, 2024
9 checks passed
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.

6 participants