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

Review REP 144 #96

Open
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
@tfoote
Member

tfoote commented Mar 1, 2015

This is an pull-request to review and then ratify REP 144. A call for comments will be issued to ros-users@ after some time we'll call for a vote. Please use this PR for comments and voting when the time is appropriate.

(For reference earlier discussions can be found in PR #94 )

@mikepurvis

This comment has been minimized.

Contributor

mikepurvis commented Mar 2, 2015

Ah, this looks great. At Clearpath, we've standardized a few more package names across our platforms, including:

  • xxx_desktop (metapackage of stuff depending on rviz, rqt, but doesn't depend on drivers)
  • xxx_viz (launchers for rviz, especially view_model.launch and view_robot.launch)
  • xxx_simulator (metapackage of stuff depending on gazebo, but otherwise has no desktop dependencies)
  • xxx_base (nodes and driver dependencies which are part of the real robot, but shouldn't be installed on the desktop or by the simulator metapackage)
  • xxx_control (nodes/plugins/launchers which are common to simulated and real robot)
@jbohren

This comment has been minimized.

jbohren commented Mar 2, 2015

Can you elaborate on:

third party libraries that are patched / integrated into ROS should not be named like their rosdep key as it creates a conflict across Ubuntu versions. If it is not specialized, name it generically <name_of_library>_ros

What about packages which are CMake only, and need to be built in order from source in a catkin workspace? In this case, creating package.xml files with the package's rosdep key as its package name makes everything build in the right order when using catkin build or catkin_make_isolated.

@wjwwood

This comment has been minimized.

Contributor

wjwwood commented Mar 2, 2015

What about packages which are CMake only, and need to be built in order from source in a catkin workspace? In this case, creating package.xml files with the package's rosdep key as its package name makes everything build in the right order when using catkin build or catkin_make_isolated.

Yeah, that's convenient, but they can never be released like that because they will conflict with the rosdep key which can install them from the package manager.

If you are building something from source because the package manager does not have it yet then I believe it should have a different name than what the rosdep key would be. We had this issue with pcl, where he had a package pcl which was our release of pcl using a package.xml and our third party pipeline and the we wanted to switch to pcl from upstream. The issue was that rosdep keys are not ROS distro specific, so you cannot have a rosdep key in one version of ROS resolve to a ROS package and in another version of ROS have that same key resolve to a system dependency. So since we say that rosdep keys should try to match the Ubuntu key, and the rosdep key cannot overlap a ROS package name even across different distros, then you shouldn't name plain CMake projects with a package.xml the same as their apt-get key name.

You can always use the same name if you never release the plain CMake package.

@mikepurvis

This comment has been minimized.

Contributor

mikepurvis commented Mar 2, 2015

Hrm. Maybe this REP should provide some specific guidance on this scenario.

For example, one of my colleagues has requested packaging a solver toolkit, and I could either just bloom third-party it, or I could set up debs in a launchpad PPA and then create a PR on reprepro-updater to pull them in, and on rosdistro to create a rosdep key for it. But it's not at all clear which is the preferred solution, and it seems like once you've decided, it's pretty painful to change your mind later.

If the REP could offer guidance into this and similar situations, that would be helpful. For example, how could the PCL situation have been handled differently to make the transition from ROS package to system package easier?

@wjwwood

This comment has been minimized.

Contributor

wjwwood commented Mar 2, 2015

The pcl example is an example of what worked (not a problem we had to solve), but I gave it as an example of why it matters. We named the new rosdep rules libpcl-1.7{-dev} which worked because the one we packaged ourselves was called pcl. However, if we had called the one we packaged libpcl-1.7, then creating a rosdep rule in the future would need a new name that didn't match Ubuntu's name. I'm just suggesting that when we roll our own versions of upstream dependencies that we do not release them with names that might collide in the future with rosdep keys. Based on this, I do not believe a third party package released with bloom should ever have the same name as you'd expect it to have in Ubuntu.

I think a recommendation of doing either a PPA or a third party bloom release is actually out of scope for this REP. So I would say that whether you release a third party package through bloom or use a PPA isn't really impacted by this guideline. You may choose to do one of those two things based on other considerations like the fact that a PPA only gets you Debian support, so you packages which depend on it will not work on RPM based systems and/or systems like OS X. But on the other hand putting it in a PPA and making a rosdep rule for it is probably a good solution if you expect it to be available in the different package managers in the future or if it is in Homebrew and/or RPM but not Debian. There are just a lot of different reasons you might choose to do one strategy over the other.

I suppose you could argue that the third party release REP could give pointers on this.

@jack-oquin

This comment has been minimized.

Contributor

jack-oquin commented Mar 2, 2015

+1 to the overal proposal. It is very helpful.

Ah, this looks great. At Clearpath, we've standardized a few more package names across our platforms, including:

xxx_desktop (metapackage of stuff depending on rviz, rqt, but doesn't depend on drivers)
xxx_viz (launchers for rviz, especially view_model.launch and view_robot.launch)
xxx_simulator (metapackage of stuff depending on gazebo, but otherwise has no desktop dependencies)
xxx_base (nodes and driver dependencies which are part of the real robot, but shouldn't be installed on the desktop or by the simulator metapackage)
xxx_control (nodes/plugins/launchers which are common to simulated and real robot)

+1 to Mike's list of additional suffixes, I suggest most of them be included in this REP.

I not sure about the last two, but could probably be persuaded.

@trainman419

This comment has been minimized.

trainman419 commented Mar 3, 2015

It would be nice to give a few examples for the prefix rules. They're pretty complicated, particularly when more than one rule applies at the same time.

Are there any rules for robot-specific configurations for other packages; for example navigation and moveit?

@mikepurvis

This comment has been minimized.

Contributor

mikepurvis commented Mar 3, 2015

@vrabaud

This comment has been minimized.

Contributor

vrabaud commented Mar 22, 2015

ok, to summarize: I need to add a few examples and that's it.

I do agree with @mikepurvis and a few others on the original thread that we also need robot package naming conventions. Here, it's only about generic ROS package. Should I complete this one ? Can it be done later ? (we can update a REP so why not). Should it be a different REP ?
It is definitely useful but also pretty hard: what is a navigation package for a robot ? Does it need to have specific services / messages ?
You can read #94 for insight, especially the ROS industrial references:
https://github.com/gavanderhoorn/rep/blob/repository_layout/rep-xxxx.rst
http://wiki.ros.org/Industrial/Tutorials/SuggestedPackageLayoutNewRepositories

@tfoote

This comment has been minimized.

Member

tfoote commented Mar 22, 2015

@vrabaud I suggest we confirm this and can have another conversation about the robot packages. It may end up with updating this REP or making a new one. But we should ratify what we agree upon before we approach the more complicated topic.

@vrabaud

This comment has been minimized.

Contributor

vrabaud commented Sep 6, 2015

before merging that, we should finish #105
Please comment there for any examples you'd like to add.

@tfoote

This comment has been minimized.

Member

tfoote commented Sep 8, 2015

I merged in #105 It contained the noteable change of moving from our (unstated) standard test_* to *_tests as the test package naming guide.

We've let this sit for quite a while does anyone object to ratifying it in the current form?

@jack-oquin

This comment has been minimized.

Contributor

jack-oquin commented Sep 8, 2015

+1 LGTM

@wjwwood

This comment has been minimized.

Contributor

wjwwood commented Sep 8, 2015

Under mandatory rules it mentions use of _, but it doesn't say anything about avoiding double underscore (__). It would be nice to get this point in, since we often use this to give us a "safe" separator when combining the package name with some other suffix or prefix in generated code. For instance, if a package called foo produces two symbols foo and foo_msg then we have no protection against another package being called foo_msg which produces foo_msg and foo_msg_msg. Having a __ be an illegal segment in the package name allows us to make foo give foo and foo__msgs which cannot be accidentally collided with.

Other than that point, I'd say +1. We should also solicit voting from ros-users@ if we haven't already.

@tfoote

This comment has been minimized.

Member

tfoote commented Sep 9, 2015

From @wjwwood I added a statement that you cannot use consecutive _ separators.

@dirk-thomas

This comment has been minimized.

Member

dirk-thomas commented Sep 9, 2015

The Mandatory Rules are pretty confusing and the rational is not always correct. I will try to rephrase the paragraph when I find time for it.

A brain dump of things which imo should be updated (if anybody want to go ahead and change it):

  • the first two bullets are not rules but definitions of terms
  • the rational for underscore is wrong - it has nothing do to with OS conventions
  • the rational for repeated underscores does not mention what generated symbols it refers to making it hard to understand what it actually means
  • the rational for the first character being alphabetic is wrong
  • explicitly mention case (must be lower case)

tfoote added a commit that referenced this pull request Oct 30, 2015

@dirk-thomas

This comment has been minimized.

Member

dirk-thomas commented Oct 10, 2017

@tfoote What is the status of this PR?

@tfoote

This comment has been minimized.

Member

tfoote commented Oct 10, 2017

It's waiting for the full form of your feedback @dirk-thomas before we circulate it broadly for ratification.

@dirk-thomas

This comment has been minimized.

Member

dirk-thomas commented Oct 10, 2017

I am sorry but I won't have time to spend on rephrasing the paragraph (especially since I neither wrote the original draft nor created this PR). Please consider using my feedback from the bullet points to update the patch if you think they should be applied.

Clarify the structure of the mandatory rules
Separate the definition.
Bold the rules to differentiate them from the explaination.
Update some rationales
@tfoote

This comment has been minimized.

Member

tfoote commented Oct 17, 2018

@dirk-thomas Please review the updated mandatory rules section

@dirk-thomas

The latest modified date should be added to the header.

Show resolved Hide resolved rep-0144.rst
Add justification for lowercase
Update post history date
@davetcoleman

This comment has been minimized.

davetcoleman commented Oct 18, 2018

this PR is 3 years old!

@peci1

This comment has been minimized.

peci1 commented Oct 18, 2018

+1

@SteveMacenski

This comment has been minimized.

SteveMacenski commented Oct 18, 2018

+1
This all seems common sense and generally as most folks have been operating for awhile.

@DLu

This comment has been minimized.

DLu commented Oct 19, 2018

+1.

Fun fact: there are three 2 letter packages: ff tf and mk

Here are the three letter packages: bfl, csm, ecl, eml, enu, fcl, gsd, joy, kni, kvh, lkh, msp, ocl, pid, rdl, ros, rqt, rtt, viz, xpp

@mintar

This comment has been minimized.

mintar commented Oct 19, 2018

+1

1 similar comment
@ross-desmond

This comment has been minimized.

ross-desmond commented Oct 19, 2018

+1

* **only consist of alphanumerics and _ separators and start with an alphabetic character.**
This allows it to be used in generated symbols and functions in all supported languages.
Lowercase is required because the names are used in directories and filenames and some

This comment has been minimized.

@130s

130s Oct 22, 2018

Can we include lowercase in the bullet point sentence, if it's a requirement? That would allow readers to grasp the idea without reading into the detail.

* **only consist of alphanumerics and _ separators and start with an alphabetic character.**
This allows it to be used in generated symbols and functions in all supported languages.
Lowercase is required because the names are used in directories and filenames and some

This comment has been minimized.

@130s

130s Oct 22, 2018

Can we include lowercase in the bullet point sentence, if it's a requirement? That would allow readers to grasp the idea without reading into the detail.

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