-
Notifications
You must be signed in to change notification settings - Fork 96
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
add meson support #691
base: master
Are you sure you want to change the base?
add meson support #691
Conversation
@wjwwood @nuclearsandwich Can you have a look at this? |
@wjwwood @nuclearsandwich @cottsay Can you please have a look at this and let me know what is missing? |
b99113d
to
dd483c0
Compare
d3231ca
to
21ce2d7
Compare
@christianrauch thank you for patiently maintaining this branch pending review. Unrelated to the quality of the proposed changes, one of the things that has been delaying my review is trying to process my thoughts on how we ought to be handling the introduction of new build types in bloom. Specifically, should we have new build types like this one for meson start out in some kind of "preview" state or give some other indication that they are relatively new and thus has different stability standards compared to the other bloom build types. I do want to stress that I'm considering a disclaimer like this around the potential for change not an expectation or assumption and around stability not quality. What I want for the meson build type and those that follow it is to decouple the overall bloom stability requirements from the ones for individual build types in order to give build type contributors/maintainers increased latitude. I would like the bloom team to be able to review the integration with bloom and trust that the build type maintainers are the subject matter experts with their tools, then just "click merge" to allow iterating in subsequent releases. What are your thoughts about something along those lines? |
It makes sense to document and/or inform users that some build types are experimental and may fail. bloom could maintain a list of experimental build types and simply show a warning "<build type> support is experimental. Please report errors to https://github.com/ros-infrastructure/bloom." when someone runs bloom to release a package. However, I also expect users of bloom to know that meson is a new build type. ROS has always been using CMake for most of its packages and now regular python processes for python packages in ROS 2. I think that someone releasing a meson package will be aware of this novelty. |
I am significantly troubled that a custom branch of bloom was used to deploy to an official rosdistro without any indication. Especially because bloom reports that the released 0.11.2 version was used for ros/rosdistro#37104 I found that ros/rosdistro#36388 (comment) references this fact. All the more reason to get this merged and released with an experimental tag. |
Sorry that this comes as a surprise to you now. Since this PR is open for more than 5 months now without getting feedback after nagging people and also since my comment on the rosdep PR (ros/rosdistro#36388 (comment)) went unnoticed, I assumed that there are no issues with the process. I am really not sure what I could have done more to notify people about this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've completed a review of the general and debian-specific changes. There aren't any serious blockers but there are a few open questions.
@cottsay would you be willing to review the RPM changes, in particular the spec file?
dh_auto_configure -- \ | ||
--prefix="@(InstallationPrefix)" \ | ||
--cmake-prefix-path="@(InstallationPrefix)" \ | ||
--libdir=lib \ | ||
--libexecdir=lib \ | ||
--strip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does meson have an equivalent to CMake's BUILD_TESTING
? #649 will exclude test dependencies conditionally based on the nocheck
debian build option. The CMake-derived build types test for this as such to specifically disable BUILD_TESTING
.
bloom/bloom/generators/debian/templates/cmake/rules.em
Lines 20 to 22 in d3d21ae
ifneq ($(filter nocheck,$(DEB_BUILD_OPTIONS)),) | |
BUILD_TESTING_ARG=-DBUILD_TESTING=OFF | |
endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see such a flag. You can directly add unit tests and run them with meson test
(https://mesonbuild.com/Unit-tests.html) but I don't see an integrated flag that could handle test dependencies. I don't know the details but I would assume that the tests are only built when you actually run meson test
.
Other than that, the test
option is commonly used which is then checked with if get_option('test')
inside the meson file. We could set that parameter here but it is not guaranteed that every meson project will use the same option for tests.
@@ -12,4 +12,4 @@ Bloom now supports different build types by inspecting the `build_type` tag in p | |||
Templates for a build type are stored in subdirectories of the platform's templates directory named for the build type. | |||
As an example the templates for the `ament_cmake` build type for debian packages is stored in [bloom/generators/debian/templates/ament_cmake](debian/templates/ament_cmake). | |||
|
|||
To add support for a new build type create a new templates subdirectory for it in your target platform's `templates` directory, and add any necessary supporting code to the generator for build type specific substitutions.For examples search for "Build-type specific substitutions" in [bloom/generators/debian/generator.py](debian/generator.py). | |||
To add support for a new build type create a new templates subdirectory for it in your target platform's `templates` directory, and add any necessary supporting code to the generator for build type specific substitutions. For examples search for "Build-type specific substitutions" in [bloom/generators/debian/generator.py](debian/generator.py). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this correction. Markdown and RST files in ros-infrastructure projects use "semantic line breaks" with one sentence per line.
So the preferred fix would be:
To add support for a new build type create a new templates subdirectory for it in your target platform's `templates` directory, and add any necessary supporting code to the generator for build type specific substitutions. For examples search for "Build-type specific substitutions" in [bloom/generators/debian/generator.py](debian/generator.py). | |
To add support for a new build type create a new templates subdirectory for it in your target platform's `templates` directory, and add any necessary supporting code to the generator for build type specific substitutions. | |
For examples search for "Build-type specific substitutions" in [bloom/generators/debian/generator.py](debian/generator.py). |
However in #702 I've expanded and rephrased this section so if this PR merges first then I'll correct it there after resolving any potential conflicts so this does not block.
I concur that you did everything in your power. It's presumptive of me to assume that you would wait indefinitely. |
ping @nuclearsandwich |
e144e15
to
667f2c5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I addressed your open questions and added more information to the commit messages.
@nuclearsandwich @cottsay Can either of you have a look at this again, please? |
The default 'installation_prefix' is '/usr'. This is wrong for 'rosrpm' and has to be prefixed correctly with the default ROS installation.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #691 +/- ##
=========================================
Coverage ? 54.59%
=========================================
Files ? 52
Lines ? 6367
Branches ? 1276
=========================================
Hits ? 3476
Misses ? 2534
Partials ? 357 ☔ View full report in Codecov by Sentry. |
2e460e8
to
0ec60c6
Compare
This adds a new
rosdebian
androsrpm
template formeson
. Fixes #690.The templates are copied from the cmake templates and the
rules.em
was adapted formeson
.Requires: #700