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

REP 149: group dependencies #141

Open
dirk-thomas opened this Issue Nov 10, 2017 · 8 comments

Comments

Projects
None yet
4 participants
@dirk-thomas
Member

dirk-thomas commented Nov 10, 2017

The draft for REP 149 contains multiple different parts. In order to not mix the discussion on those topics in a single ticket this issue is meant to focus on discussing the introduced group dependencies represented by the group_depend and member_of_group tags.

@DLu

This comment has been minimized.

Show comment
Hide comment
@DLu

DLu Nov 12, 2017

Grammar:

  • Leading and trailing whitespace is being ignoreD from
  • and are being ignoreD in the release process
  • Personally, I would find it much more clear if the phrase "a from source build" was written as "a from-source build" everywhere it is used.

I found looking at the message_generation dependencies in src most useful, and think that it would be useful to sketch how the package would be transformed if it used version 3 instead of 1.

Also, if I was to compile ros1_bridge from source would I need to manually add group_membership to all of my message packages in my workspace? What if I came up with a new msg package?

DLu commented Nov 12, 2017

Grammar:

  • Leading and trailing whitespace is being ignoreD from
  • and are being ignoreD in the release process
  • Personally, I would find it much more clear if the phrase "a from source build" was written as "a from-source build" everywhere it is used.

I found looking at the message_generation dependencies in src most useful, and think that it would be useful to sketch how the package would be transformed if it used version 3 instead of 1.

Also, if I was to compile ros1_bridge from source would I need to manually add group_membership to all of my message packages in my workspace? What if I came up with a new msg package?

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Nov 12, 2017

Member

Grammar:

  • Leading and trailing whitespace is being ignoreD from
  • and are being ignoreD in the release process
  • Personally, I would find it much more clear if the phrase "a from source build" was written as "a from-source build" everywhere it is used.

Thank you for pointing it out. Fixed in 06fa72b.

I found looking at the message_generation dependencies in src most useful, and think that it would be useful to sketch how the package would be transformed if it used version 3 instead of 1.

Please see the following comment which shows how the dependencies in a message package could look like: #138 (comment)

Also, if I was to compile ros1_bridge from source would I need to manually add group_membership to all of my message packages in my workspace? What if I came up with a new msg package?

All message packages should indeed declare being a member of the group "message_packages" (which should be done already upstream so you shouldn't need to manually do that in your workspace) . The advantage of using that approach is that if you build the bridge it will always consider all existing message packages. Without the group dependency you would need to manually add your new message package to the manifest of ros1_bridge. While such a change might be acceptable to merge into the ros1_bridge package if your message package is released it can't be accepted for not released packages. The group dependency provides a convenient way around that problem.

Member

dirk-thomas commented Nov 12, 2017

Grammar:

  • Leading and trailing whitespace is being ignoreD from
  • and are being ignoreD in the release process
  • Personally, I would find it much more clear if the phrase "a from source build" was written as "a from-source build" everywhere it is used.

Thank you for pointing it out. Fixed in 06fa72b.

I found looking at the message_generation dependencies in src most useful, and think that it would be useful to sketch how the package would be transformed if it used version 3 instead of 1.

Please see the following comment which shows how the dependencies in a message package could look like: #138 (comment)

Also, if I was to compile ros1_bridge from source would I need to manually add group_membership to all of my message packages in my workspace? What if I came up with a new msg package?

All message packages should indeed declare being a member of the group "message_packages" (which should be done already upstream so you shouldn't need to manually do that in your workspace) . The advantage of using that approach is that if you build the bridge it will always consider all existing message packages. Without the group dependency you would need to manually add your new message package to the manifest of ros1_bridge. While such a change might be acceptable to merge into the ros1_bridge package if your message package is released it can't be accepted for not released packages. The group dependency provides a convenient way around that problem.

@DLu

This comment has been minimized.

Show comment
Hide comment
@DLu

DLu Nov 16, 2017

Please see the following comment which shows how the dependencies in a message package could look like: #138 (comment)

I was suggesting that something like that be actually incorporated into the REP.

All message packages should indeed declare being a member of the group "message_packages"

On a more fundamental level, why can't any package that uses the add_message_files command automatically be added to the group message_packages

DLu commented Nov 16, 2017

Please see the following comment which shows how the dependencies in a message package could look like: #138 (comment)

I was suggesting that something like that be actually incorporated into the REP.

All message packages should indeed declare being a member of the group "message_packages"

On a more fundamental level, why can't any package that uses the add_message_files command automatically be added to the group message_packages

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Nov 16, 2017

Member

On a more fundamental level, why can't any package that uses the add_message_files command automatically be added to the group message_packages

The group membership needs to be readable / parseable without evaluating e.g. CMake code. Therefore it is being declared in the manifest. So this can't be declared automatically by any CMake function.

Member

dirk-thomas commented Nov 16, 2017

On a more fundamental level, why can't any package that uses the add_message_files command automatically be added to the group message_packages

The group membership needs to be readable / parseable without evaluating e.g. CMake code. Therefore it is being declared in the manifest. So this can't be declared automatically by any CMake function.

@scpeters

This comment has been minimized.

Show comment
Hide comment
@scpeters

scpeters Nov 16, 2017

On a more fundamental level, why can't any package that uses the add_message_files command automatically be added to the group message_packages

The group membership needs to be readable / parseable without evaluating e.g. CMake code. Therefore it is being declared in the manifest. So this can't be declared automatically by any CMake function.

I think a linter rule could be added to remind people to add that statement to the manifest if the add_message_files command is detected.

scpeters commented Nov 16, 2017

On a more fundamental level, why can't any package that uses the add_message_files command automatically be added to the group message_packages

The group membership needs to be readable / parseable without evaluating e.g. CMake code. Therefore it is being declared in the manifest. So this can't be declared automatically by any CMake function.

I think a linter rule could be added to remind people to add that statement to the manifest if the add_message_files command is detected.

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Nov 16, 2017

Member

I think a linter rule could be added to remind people to add that statement to the manifest if the add_message_files command is detected.

It would indeed be great if someone would write a linter for this 👍

Member

dirk-thomas commented Nov 16, 2017

I think a linter rule could be added to remind people to add that statement to the manifest if the add_message_files command is detected.

It would indeed be great if someone would write a linter for this 👍

@esteve

This comment has been minimized.

Show comment
Hide comment
@esteve

esteve Dec 4, 2017

Contributor

Would this accommodate external language generators? @firesurfer and I have to distribute patches for rosidl_default_generators to include our generators, but it'd be great if they were automatically injected if they are declared as member_of_group of rosidl_default_generators. Is this possible with the current draft? If so, I can work on adding that logic to the rosidl_default_generators package

Contributor

esteve commented Dec 4, 2017

Would this accommodate external language generators? @firesurfer and I have to distribute patches for rosidl_default_generators to include our generators, but it'd be great if they were automatically injected if they are declared as member_of_group of rosidl_default_generators. Is this possible with the current draft? If so, I can work on adding that logic to the rosidl_default_generators package

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Dec 4, 2017

Member

@esteve Yes, that should be useful for that use case too. I created a ticket to track that feature idea: ros2/rosidl_typesupport#17.

Member

dirk-thomas commented Dec 4, 2017

@esteve Yes, that should be useful for that use case too. I created a ticket to track that feature idea: ros2/rosidl_typesupport#17.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment