search

ipa320 / care-o-bot

http://www.care-o-bot.de
Apache License 2.0
48 stars 41 forks source link

[travis] improvements and extensions #45

Open fmessmer opened 7 years ago

fmessmer commented 7 years ago

@ipa-fez @ipa-jba @ipa-rmb @ipa-fmw @ipa-mig @ipa-nhg @ipa-mdl @fmessmer FYI

I'll start this issue for discussion that should lead to an improved/extended default .travis.yml that we use in all our repos (see list below). I suggest to first discuss which new features are to be added to the new default .travis.yml and then provide PRs for all affected repos in a torrent for consistency!

New features to be added:

Who steps forward and is willing to provide a suggestive PR against care-o-bot, i.e. this, repo?

NAV-REPOS:

PERCEPTION-REPOS:

MSH-REPOS:

Please add repos in case I missed one...

mathias-luedtke commented 7 years ago

enable compiler warnings as errors (as done by @ipa-fez e.g. in ipa320/ipa_navigation_driver#40)

It is okay to add it to travis as well, but I recommend to set this for every (c++) package. This way the user gets the error immediately.

ipa-fez commented 7 years ago

It is okay to add it to travis as well, but I recommend to set this for every (c++) package. This way the user gets the error immediately.

Do you mean directly in the CMakeLists.txt @ipa-mdl?

ipa-rmb commented 7 years ago

Though I like getting rid of warnings, I fear we cannot eliminate all warnings that originate from PCL and we have no means to fix that (multiple dependencies to other libraries). Hence I would suggest that everyone who can afford turning this on, should do it in cmake.

mgruhler commented 7 years ago

@ipa-mdl Do you know of a way that catkin_lint does not treat this as an error? I.e. not changing the CMAKE_CXX_FLAGS?

@ipa-fmw Additionally I only see unit tests...

mathias-luedtke commented 7 years ago

@ipa-mig: Please, don't mess with CMAKE_CXX_FLAGS in CMakeLists.txt; catkin_lint might even warn you (if not, this should be added). Per-package options should be set with add_compile_options (please set cmake_minimum_required(VERSION 2.8.12)). Alternative for CMake < 2.8.12 (e.g. < ubuntu 14.04): add_definitions

@ipa-rmb: The warnings we'd like to address should not occur in PCL or any other common library.

ipa-rmb commented 7 years ago

Ah yes, these warnings should probably not occur and are serious errors.

I agree with removing jade.

mgruhler commented 7 years ago

@ipa-mdl I had no intention to do so. As I said, catkin_lint treats this as an error.

I forgot about add_compile_options. Shame on me ;-)

-------- Originalnachricht -------- Betreff: Re: [ipa320/care-o-bot] [travis] improvements and extensions (#45) Von: Mathias Lüdtke notifications@github.com An: ipa320/care-o-bot care-o-bot@noreply.github.com Cc: "Gruhler, Matthias" matthias.gruhler@ipa.fraunhofer.de,Mention mention@noreply.github.com

@ipa-mighttps://github.com/ipa-mig: Please, don't mess with CMAKE_CXX_FLAGShttp://docs.ros.org/jade/api/catkin/html/user_guide/standards.html in CMakeLists.txt; catkin_lint might even warn you (if not, this should be added). Per-package options should be set with add_compile_optionshttps://cmake.org/cmake/help/v3.0/command/add_compile_options.html (please set cmake_minimum_required(VERSION 2.8.12)). Alternative for CMake < 2.8.12 (e.g. < ubuntu 14.04): add_definitionshttps://cmake.org/cmake/help/v3.0/command/add_definitions.html

@ipa-rmbhttps://github.com/ipa-rmb: The warnings we'd like to address should not occur in PCL or any other common library.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/ipa320/care-o-bot/issues/45#issuecomment-318609388, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ABT3_Gyh6I5h6a7ZXZdmKPzqIYC27ehxks5sSa0lgaJpZM4Oj2eM.

fmessmer commented 7 years ago

most packages done @ipa-rmb took care of perception repos I'll leave nav repos to @ipa320/navigation-push

mathias-luedtke commented 7 years ago

@ipa-fxm: if you don't set ROS_REPOSITORY_PATH or ROS_REPO, the shadow-fixed repository will be used.

fmessmer commented 7 years ago

if you don't set ROS_REPOSITORY_PATH or ROS_REPO, the shadow-fixed repository will be used.

I did set ROS_REPO=ros in https://github.com/ipa320/care-o-bot/pull/47/files#diff-354f30a63fb0907d4ad57269548329e3R10....or did I miss it somewhere?

mathias-luedtke commented 7 years ago

@ipa-fxm: Just had a brief look and missed that you have set it globally. Sorry for the noise!

It might make sense to use shadow-fixed by default, to detect upstream breaks earlier. I am not sure which way to go..

fmessmer commented 7 years ago

debian jobs do not make too much sense for private repos like ipa_navigation and msh as they depend on other private packages that might not been released (too soon/ever)... still I kept the syntax improvements and ROS_REPO part for msh... I leave it to @ipa320/navigation-push to decide about ipa_navigation