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

[travis] improvements and extensions #45

Open
fmessmer opened this issue Jul 26, 2017 · 12 comments
Open

[travis] improvements and extensions #45

fmessmer opened this issue Jul 26, 2017 · 12 comments

Comments

@fmessmer
Copy link
Member

fmessmer commented Jul 26, 2017

@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
Copy link

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
Copy link

ipa-fez commented Jul 28, 2017

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
Copy link

ipa-rmb commented Jul 28, 2017

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
Copy link
Member

@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
Copy link

@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
Copy link

ipa-rmb commented Jul 28, 2017

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

I agree with removing jade.

@mgruhler
Copy link
Member

mgruhler commented Jul 29, 2017 via email

@fmessmer
Copy link
Member Author

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

@mathias-luedtke
Copy link

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

@fmessmer
Copy link
Member Author

fmessmer commented Jul 31, 2017

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
Copy link

@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
Copy link
Member Author

fmessmer commented Aug 1, 2017

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

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

No branches or pull requests

5 participants