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.

Sign up for GitHub

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

Jump to bottom

Migration to package format 2 #7

Closed
fmessmer opened this issue Aug 11, 2015 · 28 comments
Closed

Migration to package format 2 #7

fmessmer opened this issue Aug 11, 2015 · 28 comments

Comments

@fmessmer
Copy link
Member

@ipa-fmw @ipa-nhg @ipa-mdl @ipa-mig @ipa-rmb @ipa-bnm

https://github.com/pulls?utf8=%E2%9C%93&q=is%3Aopen+is%3Apr+user%3Aipa320+Migration+package+format+2

Let's use this Issue for general discussion about what's still missing for a successfull migration to package format 2:

Remaining repos to be migrated:

  • cob_extern
  • cob_perception_common
  • cob_perception_data
  • cob_navigation
  • cob_android
  • care-o-bot (this package)
  • ...

Remaining decisions to be applied wherever it occurs:

  • Are we going to install headers for "executable-only" packages?
  • ...

Please contribute to the discussion in case something is not yet mentioned here...
@mathias-luedtke
Copy link

+1 for not installing headers for"executable-only" packages

@mathias-luedtke
Copy link

  • tests and roslaunch checks, test_depends

@mgruhler
Copy link
Member

+1 for not installing headers of "executable-only" packages.

@mgruhler
Copy link
Member

How should boost dependencies be handled?
find_package it when used directly, maybe with COMPONENTS REQUIRED?
How should it be in the package.xml?

@mgruhler mgruhler mentioned this issue Aug 13, 2015
Merged
@benmaidel
Copy link
Member

+1 for not installing headers of "executable-only" packages

@fmessmer
Copy link
Member Author

I'd suggest to be consequent and add explicit dependencies for boost!
I also already started to clean-up executable-only packages (see cob_command_tools)

@fmessmer
Copy link
Member Author

Also, how about packages where there are libraries?
In case the libraries are only used within the same packages, they do not need to be exported, right?

This would mean:

  • no install tag for the library?
  • no install tags for the headers used in the library?
  • no LIBRARIES in catkin_package?
  • no INCLUDE_DIRS in catkin_package?
  • thus, also no CATKIN_DEPENS and DEPENDS required?
    Is this correct?

How about libraries, that are used by other packages, e.g. cob_generic_can or cob_canopen_motor?

  • install library!
  • export LIBRARIES in catkin_package!
  • install headers, too?

What about plugins?
E.g. cob_omni_drive_controller?

@ipa-mdl could you bring in some light here?

@fmessmer fmessmer mentioned this issue Aug 13, 2015
Open
@fmessmer
Copy link
Member Author

I'd postpone reviewing "everything-tests-related" to the new issue #8

@fmessmer
Copy link
Member Author

What are we going to to about tags like this (e.g. from here):

if(CMAKE_COMPILER_IS_GNUCXX)
  add_definitions(-std=gnu++0x)
else()
  add_definitions(-std=c++0x)
endif()

@mathias-luedtke
Copy link

You are right with the libraries.
Plug-ins have to be built and installed, headers are not needed. And the plugin xml has to be installed, of cause.

C++11 code should be avoided for now if possible.
I had a brief look at cob_base_velocity_smoother, I think it does not need c++11.

C++11 is will be okay for executables, but probably not for librares, since the c++11 flags must be passed to dependent packages and I do not know if this works.

@mgruhler
Copy link
Member

So we have to decide how to handle the libraries, right?
IMO, installing libraries and headers will allow other people to use this functionality.
Will also make things easier, if someone from IPA starts working on a node using a library, that is in another package.

So my vote is for installing (generally) installing libraries and headers with the respective CATKIN_DEPEND with the exception of where it will definitely not be used somewhere else (e.g. cob_undercarriage_ctrl due to getting deprecated)

@mgruhler
Copy link
Member

So boost should only be addes as dependency, when any boost header is explicitely included. right?

@mathias-luedtke
Copy link

Boost should be added at least if certain components are used (system, threads, file_system).

If the libs should be used elsewhere (i.e. are useful for others, too) they should be installed.
Each lib needs its headers installed!

@fmessmer
Copy link
Member Author

So most of the packages have been migrated and merged!

Remaining repos to be migrated:

  • cob_extern
  • cob_perception_common
  • cob_perception_data
  • cob_navigation
  • cob_android
  • care-o-bot (this package)
  • ...

@ipa-fmw @ipa-nhg @ipa-mdl @ipa-mig @ipa-rmb @ipa-bnm
So, who is volunteering to migrate the packages above?

@mgruhler
Copy link
Member

I'll take over cob_navigation. And I can also do cob_extern.

@benmaidel
Copy link
Member

i take cob_android

@ipa-nhg
Copy link
Member

ipa-nhg commented Aug 17, 2015

I take care-o-bot and cob_test_robots

@benmaidel
Copy link
Member

and my votes
libraries:

  • always install libraries with their headers even if the lib is only used inside the same package for the moment
  • generally export libraries and headers in catkin_package

plugins:

  • as mdl already wrote. Install plugin.xml and the lib. No need for headers!

boost:

  • i think boost depends are only needed if certain non header-only libs are used. Here a list of boost libs that require building:
    Boost.Chrono,
    Boost.Context,
    Boost.Filesystem,
    Boost.GraphParallel,
    Boost.IOStreams,
    Boost.Locale,
    Boost.MPI,
    Boost.ProgramOptions,
    Boost.Python,
    Boost.Regex,
    Boost.Serialization,
    Boost.Signals,
    Boost.System,
    Boost.Thread,
    Boost.Timer,
    Boost.Wave

@fmessmer
Copy link
Member Author

@ipa-bnm
That's the way it has been done now for the packages that are already migrated....

@mgruhler
Copy link
Member

ipa320/cob_navigation#50 and ipa320/cob_extern#48 done.

@fmessmer
Copy link
Member Author

Awesome
👍

@fmessmer
Copy link
Member Author

I'll start triggering new releases...later the day/evening...@ipa-fmw @ipa-nhg FYI

@mgruhler
Copy link
Member

I’d like to do this for cob_navigation as well.
Can you maybe show me how to do this some time this/next week?

Von: Felix Messmer [mailto:notifications@github.com]
Gesendet: Dienstag, 25. August 2015 14:45
An: ipa320/care-o-bot
Cc: Gruhler, Matthias
Betreff: Re: [care-o-bot] Migration to package format 2 (#7)

I'll start triggering new releases...later the day/evening...

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/7#issuecomment-134573026.

@fmessmer
Copy link
Member Author

New Releases triggered for Indigo:

Did I forget something?

@fmessmer fmessmer mentioned this issue Aug 25, 2015
Closed
@fmessmer
Copy link
Member Author

I decided to go on with the rest of the repositories...
...as releasing can easily be done, we could release in shorter cycles whenever a new fix is introduced within one of the repos!

@fmessmer
Copy link
Member Author

fmessmer commented Sep 2, 2015

All 💚 😉

@mgruhler
Copy link
Member

mgruhler commented Sep 2, 2015

👏

@floweisshardt
Copy link
Member

schunk_robots will not be released

@fmessmer fmessmer mentioned this issue Nov 20, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@floweisshardt @benmaidel @fmessmer @ipa-nhg @mathias-luedtke @mgruhler