Closed wjwwood closed 9 years ago
Ok, so initially my response was going to read "Is there some way to simply get rid of CMAKE_BUILD_TYPE=None being part of the config?". Then I read around a couple of tickets and bug reports and it looks like the intended usage here is very much part of the debian packaging standards, the idea being that they want to cancel out the CMAKE flags and define only the ones that come from dpkg-buildflags (ex: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=730636). If this is the upstream standard, we should probably stick to it.
So, now we come back to the several recent examples of issues. And after looking at them, I don't think we should actually change anything with bloom -- but we should make developers more aware of things (for instance, there was serious confusion at the idea that debians weren't being built in release, and that asserts were still active). In the case of the MoveIt issue, that assert probably shouldn't have even been there to begin with -- and if the developer knew that asserts would be active in debs, they probably wouldn't have used an assert there. I'm also not convinced that -03 vs -02 is much of a problem with MoveIt (certainly, lack of -02 is a huge issue as we found out in the early days of Indigo). If we do find that -03 is needed, it could be set manually for the packages that need it (certainly, not all of MoveIt needs it)
In regards to RVIZ however, one thing that this discussion does bring up is that we probably shouldn't be patching the bloom configs, but rather the CMake itself (so that others who come along and package for new systems don't improperly package it). I imagine in the case of the -DNDEBUG, the proper thing would be to have a block in CMake that sets -DNDEBUG if CMAKE_BUILD_TYPE != Debug.
Some other links I came across while reviewing this:
FYI, the current RPM .spec
files utilize the system's default cmake
invocation. This expands (after changing the ROS-specific vars) to look something like:
CFLAGS="${CFLAGS:--O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -m64 -mtune=generic}" ; export CFLAGS ;
CXXFLAGS="${CXXFLAGS:--O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -m64 -mtune=generic}" ; export CXXFLAGS ;
FFLAGS="${FFLAGS:--O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -m64 -mtune=generic -I/usr/lib64/gfortran/modules}" ; export FFLAGS ;
FCFLAGS="${FCFLAGS:--O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -m64 -mtune=generic -I/usr/lib64/gfortran/modules}" ; export FCFLAGS ;
LDFLAGS="${LDFLAGS:--Wl,-z,relro }" ; export LDFLAGS ;
/usr/bin/cmake \
-DCMAKE_C_FLAGS_RELEASE:STRING="-DNDEBUG" \
-DCMAKE_CXX_FLAGS_RELEASE:STRING="-DNDEBUG" \
-DCMAKE_Fortran_FLAGS_RELEASE:STRING="-DNDEBUG" \
-DCMAKE_VERBOSE_MAKEFILE:BOOL=ON \
-DCMAKE_INSTALL_PREFIX:PATH=/usr \
-DBUILD_SHARED_LIBS:BOOL=ON \
-DCMAKE_INSTALL_PREFIX=/opt/ros/indigo \
-DCMAKE_PREFIX_PATH=/opt/ros/indigo \
-DSETUPTOOLS_DEB_LAYOUT=OFF \
-DCATKIN_BUILD_BINARY_PACKAGE="1"
Worth noting is that -DNDEBUG
is present by default, and -O2
appears to be the system's default optimization level. I'm not saying that Ubuntu should do the same thing, but I thought that this would be useful information.
I haven't seen any problems with this build configuration, but there certainly hasn't been any widespread testing of the compiled packages. I can, however, verify that there have been no compilation problems related to -DNDEBUG
that have come across my desk (meaning that as of 2 weeks ago, no packages in Indigo or Hydro presented any issues).
Something I think would be worth mentioning along these lines is the use of BUILD_SHARED_LIBS
on the cmake
call. From what I can tell, this flag isn't set in either source builds or Ubuntu builds. The only package I've seen that it seems to make a difference in was an upstream package that made assumptions about the library names that needed to be corrected (and is fixed now to handle BUILD_SHARED_LIBS
...I can't remember the name of the package, but I could go find it if need be See [1]). Should this flag be set on the Ubuntu builds as well, to ensure .so
libraries are built and not .a
static ones? What about source builds?
--scott
EDIT: Related .spec
template section [2]
[1] https://github.com/uji-ros-pkg/uwsim_bullet/pull/1 [2] https://github.com/ros-infrastructure/bloom/blob/master/bloom/generators/rpm/templates/template.spec.em#L23-L32
I don't have an strong opinion about how to proceed. Some thoughts:
buildsystem=cmake
in your rules files on Debian, you will get CMAKE_BUILD_TYPE=None
by default.-DNDEBUG
will only affect to assertions. Leaving the assertions enabled or disabled in production code has been an open debate in the testing community for years. Obviously if it impacts heavily in the performance it needs to be disabled, otherwise it could be useful to leave them enabled. We should not see NDEBUG
always associated to all non Debug
types.I strongly favor option 4 ("do nothing in bloom, but write up how to explicitly opt-into these flags"), and sticking as closely as possible to the Debian policies. This will make things easier if we ever want to get parts of ROS into Debian/Ubuntu; also, there's a reason why Debian didn't pick -O3
as a default.
So I suggest we stick with Debian policy 10.1 (use -O2
by default, and let the package maintainer decide to use -O3
selectively):
It is up to the package maintainer to decide what compilation options are best for the package. Certain binaries (such as computationally-intensive programs) will function better with certain flags (-O3, for example); feel free to use them. Please use good judgment here. Don't use flags for the sake of it; only use them if there is good reason to do so.
@jspricke
I prefer that -O3 only be set on a per-package basis, following adequate testing by the maintainer.
I think we can all agree that -O3
is not a good idea, and therefore forcing -DCMAKE_BUILD_TYPE=Release
is not a good idea.
So the question still remains about NDEBUG
. Is it harmless enough to turn on for everyone; is it beneficial enough to take the risk? It was extremely important for rviz
, but that's probably anecdotal.
If you use buildsystem=cmake in your rules files on Debian, you will get CMAKE_BUILD_TYPE=None by default.
We do pass that directly, which creates some unknown option: buildsystem=cmake
warnings, but that's a catch all to prevent the auto-detection of the buildsystem from detecting an automake package wrapped with CMake as automake in the rules file. In most cases it should auto-detect to cmake anyways.
@cottsay's comment seems to suggest that adding NDEBUG
would be OK.
Something I think would be worth mentioning along these lines is the use of BUILD_SHARED_LIBS on the cmake call.
We don't set it because it seems to defaults to shared. Though I've actually seen some things which indicate the opposite. catkin
doesn't explicitly set BUILD_SHARED_LIBS
to ON
as far as I can tell:
https://github.com/ros/catkin/search?utf8=%E2%9C%93&q=BUILD_SHARED_LIBS
But does note that it assumes that it defaults to ON
. All that being said, I guess we could set it explicitly for completeness.
In regards to RVIZ however, one thing that this discussion does bring up is that we probably shouldn't be patching the bloom configs, but rather the CMake itself (so that others who come along and package for new systems don't improperly package it). I imagine in the case of the -DNDEBUG, the proper thing would be to have a block in CMake that sets -DNDEBUG if CMAKE_BUILD_TYPE != Debug.
That may be the case, but I'm not sure that's the best option if all packages must/should do this. With respect to others packaging ROS, I think that having a clear description on what flags should be passed for packaging on Ubuntu and Fedora would be the best indication to them on how they should package ROS packages.
Also consider that you may want debug messages from debug macros in Release mode if you are building from source. I understand the inclination to move this to the source code, but maybe it has a place at the packaging level. The problem becomes, as a packager, if I want to build rviz with -DCMAKE_BUILD_TYPE=None
and without passing -DNDEBUG
then I would have to patch the CMake code of rviz. Ideally, you would need some sort of -DPACKAGE_DO_NOT_SET_NDEBUG
so that from the outside you could undo your optimization which sets -DNDEBUG
if CMAKE_BUILD_TYPE != Debug
.
So I'm sort of not sure what the best approach would be here, but my guess would be to leave -DNDEBUG
as an option to be set externally from CMake. A compromise might be to have a common package set this flag when not Debug but have sufficient visibility and options to configure it to any use case, though this leads off the beaten path and adds more "magic" to the system.
catkin
sets BUILD_SHARED_LIBS
explicitly (if not overridden by the user): https://github.com/ros/catkin/blob/2599eb973762c2a23f103fdc8bd0ef40e779f2ed/cmake/tools/libraries.cmake#L12
Lesson learned, don't trust GitHub's search. Thanks for the correction @dirk-thomas.
Perhaps turn on -DNDEBUG
in the next release of ROS?
Thanks, @dirk-thomas. I'd suppose the problem presented itself because the package in question is a plain cmake
package. More or less, I was wondering if it should be set to be sure that the non-catkin packages default to producing shared libs...
As for the -DNDEBUG
issue, would it be safe to say that anyone who would want the extra verbosity produced because of -DNDEBUG
and would be able to actually use it is probably capable of building the package from source? Assuming -DNDEBUG
is added at packaging time (in Bloom), source builds would continue as they are (notably without -DNDEBUG
).
I think the answer to this question is part of the answer to the question "Who or what are the binary .deb/.rpm packages intended for?" I've always sort of thought that they were intended for those not developing on the package, but rather for those using it, who expect performance over debug-ability. If this flag is known to adversely affect performance, and is not known to adversely affect widespread compilation, it sounds like it would be better to add it to all .deb/.rpm by default.
I see merit in going either way, so I can't really say which one I think is better, but hopefully I can help make an informed decision on this :)
Also, the discussion seems to be leaning away from -O3
. Is the alternative -O2
or no optimization by default (Or is this yet to be determined...)?
More or less, I was wondering if it should be set to be sure that the non-catkin packages default to producing shared libs...
I don't think we should be setting CMake's BUILD_SHARED_LIBS
option for non-catkin packages. That is a product of the legacy of how our packages were written and the fact that rosbuild set it explicitly, and so it persists now. However, for plain cmake packages I think they should be responsible for getting their flags right without help from outside. Basically, I wouldn't choose to add that flag to all cmake packages in Ubuntu, so I wouldn't for all packages on our farm either.
As for the -DNDEBUG issue, would it be safe to say that anyone who would want the extra verbosity produced because of -DNDEBUG and would be able to actually use it is probably capable of building the package from source?
I tend to agree, I think that NDEBUG
has performance benefits which out weigh the arguments against it and developers can not only choose to include or exclude it when building from source, but they can also opt-out of it with a patch to their release repository, just like people can opt-in to -O3
.
Also, the discussion seems to be leaning away from -O3. Is the alternative -O2 or no optimization by default (Or is this yet to be determined...)?
I believe the change in https://github.com/ros-infrastructure/bloom/pull/278 brought us automatic flags from dpkg-buildflags
which should include -O2
, which is why we suggested people re-bloom their packages after that ticket to resolve performance related issues.
To clarify: -DNDEBUG
by design, will give you (99% of the cases) equal or more performance than not using it, depending on how many assertion there are in the code and, specially, what are they checking and how. Any package failing to compile when using -DNDEBUG
can be consider broken, assertions should never affect the program logic. The only trade off is to miss the ability of leaving assertions enabled on production code, but I think that the ROS community can live with that.
Speaking about -O2
, there are probably a good number of people who don't know/care about it and given the increase on performance against -O0, I would consider it as an improvement. I'm yet to see a code broken by -O2 which is not buggy itself.
To support my -O2
claim, I pulled this from the most recent roscpp
job on Hydro (http://jenkins.ros.org/job/ros-hydro-roscpp_binarydeb_precise_amd64/38/consoleFull):
/usr/lib/ccache/c++
-Droscpp_EXPORTS
-DROS_PACKAGE_NAME=\"roscpp\"
-DROSCONSOLE_BACKEND_LOG4CXX
-g
-O2
-fstack-protector
--param=ssp-buffer-size=4
-Wformat
-Wformat-security
-Werror=format-security
-fPIC
-I/tmp/buildd/ros-hydro-roscpp-1.10.11-0precise-20141016-1507/obj-x86_64-linux-gnu/devel/include
-I/tmp/buildd/ros-hydro-roscpp-1.10.11-0precise-20141016-1507/include
-I/tmp/buildd/ros-hydro-roscpp-1.10.11-0precise-20141016-1507/obj-x86_64-linux-gnu/devel/include/ros
-I/opt/ros/hydro/include
-I/tmp/buildd/ros-hydro-roscpp-1.10.11-0precise-20141016-1507/obj-x86_64-linux-gnu
-o
CMakeFiles/roscpp.dir/src/libros/common.cpp.o
-c
/tmp/buildd/ros-hydro-roscpp-1.10.11-0precise-20141016-1507/src/libros/common.cpp
And the -O2
flag is in there, but -DNDEBUG
is not. If you look at a similar job for rviz you see the same thing except it has the -DNDEBUG
because I explicitly added it to the rules file in the release repository.
And after looking at them, I don't think we should actually change anything with bloom -- but we should make developers more aware of things (for instance, there was serious confusion at the idea that debians weren't being built in release, and that asserts were still active).
Could you explain (or document somewhere) how developers can set:
DEB_CXXFLAGS_MAINT_APPEND=-DNDEBUG
for the debian releases of their individual packages (similar to how Rviz does it)? I'm not sure where to put the flag.
I will document it when we come to a decision about how to proceed here. If there are any more comments people have please make them soon, I will likely make a decision on this within the day.
@davetcoleman In the mean time you can set it as a patch to the debian/<rosdistro>/<package>
branch in the debian/rules.em
file, for example this is where I did it in rviz
's release repository:
https://github.com/ros-gbp/rviz-release/commit/66f3d90b6e2cf536192ea07eb8015edda3f3d0d7
Ok, we've made a decision on this:
CMAKE_BUILD_TYPE
, so it will remain the default None
-O2
will be used.-DNDEBUG
flag by default in the rules file.So, new packages and new releases of packages will get the -DNDEBUG
flag turned on. I will write new documentation to detail the fact that we do set this by default and how to opt out of this flag to go back to the current behavior. I'll also document how to opt in to things like -O3
. I'll then release bloom and send out an announcement to ros-users.
So, if you see a problem with this, speak now.
Thanks for the input everyone.
I'm going to release the change to enable this policy change today, see #338.
I have also updated the tutorials to include a tutorial about how to opt in/opt out of the different flags. The tutorial is pretty primitive and light on reasoning about why you might change the flags, but it's all I have time for at the moment. If anyone is interested in fleshing it out a bit I would appreciate the help.
http://wiki.ros.org/bloom/Tutorials/ChangeBuildFlags
I'll make the release today and make an announcement afterwards to ros-users about the change.
Regarding the new wiki page http://wiki.ros.org/bloom/Tutorials/ChangeBuildFlags:
$ cd <folder you checked it out to>
$ git checkout release/<rosdistro>/<package_name>
Shouldn't the changes to the rules.em file be made in the debian/<rosdistro>/<package_name>
branch? The release/<rosdistro>/<package_name>
branch does not have the debian folder yet.
$ git commit -am <some message about how you changed the flags>
$ git push
Is it necessary to call git-bloom-patch export
after having committed the new version (as explained on the ReleaseThirdParty page for adding a package.xml
file)?
Shouldn't the changes to the rules.em file be made in the
debian/<rosdistro>/<package_name>
branch?
Yes! Thanks for pointing that out, I just update the page.
Is it necessary to call git-bloom-patch export after having committed the new version (as explained on the ReleaseThirdParty page for adding a package.xml file)?
No, I believe not, because the release process will actually do that before processing/update that branch. IIRC, I made this change to make it possible to edit a file through the github interface and then just run bloom-release
.
We should double check that but either way running git-bloom-patch export
will not hurt anything.
Thanks for the documentation!
So, I'm not sure we want to reopen this discussion at the moment but debian policy calls for -O2
and -g
by default with -O3
available to use by packages managers. Debian Policy 10.1
@tfoote : So, what changes to the current state are you suggesting? Adding -g
and/or removing -DNDEBUG
? The debian policy says nothing about NDEBUG
.
Upon review I think that we're compliant so we don't need to do anything. Adding RelWithDebInfo was proposed briefly in #374 and the link to the debian policy came up, which was not previously linked in this thread so I wanted to add it for reference. But we look to already have -g
-O2
and we should keep the -DNDBUG
so we're compliant as is.
This currently applies to Ubuntu/Debian but could also affect others by way of common policy. Currently we do not explicitly set any release related compiler or CMake flags like
-DNDEBUG
,-O3
, or-DCMAKE_BUILD_TYPE=Release
, the question is should we and if so which ones?tl;dr:
As I see it we can choose one of these options:
-DCMAKE_BUILD_TYPE=Release
(effectively-O3 -DNDEBUG
) to the bloom template-NDEBUG
-O3
I am leaning towards the third option, but I would like some feedback @dirk-thomas @tfoote @mikeferguson @isucan @davetcoleman @mintar @tlind @j-rivero
Related tickets:
Currently we pass some non-release related flags through
deb_helper
:https://github.com/ros-infrastructure/bloom/blob/master/bloom/generators/debian/templates/rules.em#L27-30
The
CATKIN_BUILD_BINARY_PACKAGE
CMake argument is how we disable installing ofsetup.bash
.My understanding (given the research I have done into things) is that
deb_helper
inserts default values into the build, for both CMake and the compiler and some of those defaults come fromdpkg-buildflags
. This happens automatically as of debhelper version 9 (which bloom is already using, see: https://github.com/ros-infrastructure/bloom/pull/278), see: https://wiki.debian.org/HardeningWalkthrough#Enabling_dpkg-buildflags_in_your_debian.2Frules_filesIn accordance with this,
deb_helper
prepends our CMake variables with-DCMAKE_BUILD_TYPE=None
. This (I think) prevents CMake from overriding by default the flagsdeb_helper
passes to the compiler. We could override this by adding-DCMAKE_BUILD_TYPE=Release
in the bloom rules template linked above, however, I think this potentially dangerous.From my experience the
-DCMAKE_BUILD_TYPE=Release
essentially boils down to-O3
and-DNDEBUG
on our Ubuntu buildfarm, see: https://github.com/ros-visualization/rviz/issues/775#issuecomment-59185412I would consider
-O3
too aggressive, because it can actually affect the behavior of code, not just the performance, so changing from opt-in to opt-out on that one will potentially break packages for people who have not explicitly checked this. But then again, this is more likely to have more utility than for the many packages it could speed up than the few packages it might break. Either way it is hard to estimate if it is a net win or loss to add-O3
for all packages automatically.On the other hand,
-DNDEBUG
should not exclude code which affects the behavior of a system, but still could potentially be bad if the developer of a package did something odd like defining a variable inside the debug area and using it outside. Given that caveat I would feel better about adding-DNDEBUG
automatically to all packages (requiring them to opt-out explicitly if desired) than doing the same for-O3
.