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

revamped roslaunch $(find) stuff #228

Closed
wants to merge 1 commit into from
Closed

Conversation

dirk-thomas
Copy link
Member

  • modified roslaunch $(find PKG) to consider path after it for resolve strategy
  • add separate commands:
    • $(find-executable PKG PATH)
    • $(find-share PKG PATH)
    • $(find-share-devel PKG)
    • $(find-share-source PKG)

Update - the following commands have been removed again:

  • $(find-share-devel PKG)
  • $(find-share-source PKG)

Update - the following command has been renamed:

  • $(find-share PKG PATH) -> $(find-resource PKG PATH)

Update:

@dirk-thomas
Copy link
Member Author

@tfoote @wjwwood Please review.

@dirk-thomas
Copy link
Member Author

Some more information about how it works (this must be documented on the roslaunch wiki page):

Since $(find PKG) can not resolve to a reasonable path without know what it is used for it has to consider the path following after the closing parenthesis. This is pretty ugly but otherwise that command could not be "fixed".

Therefore a set of new commands has been added:

  • $(find-executable PKG PATH): will resolve executables, it will first check for lib/PKG/basename(PATH) and than for PKG_SOURCE_FOLDER/PATH
  • $(find-share PKG PATH): will resolve files from share (devel will be prefered over source, while source is equal to the package folder), share/PKG/PATH
  • $(find-share-devel PKG): will resolve to the share folder of PKG in either install space or devel space
  • $(find-share-source PKG)will resolve to the share folder of PKG in either install space or source space (aka the pkg source folder)

All new commands will error out when the searched path is not found.

For backward compatibility the command $(find PKG)PATH will act as follows:

  • try find-executable PKG PATH
  • if the previous command fails try find-share PKG PATH
  • if the previous command fails resolve find-share-source PKG

@wjwwood
Copy link
Member

wjwwood commented May 17, 2013

+1, cursory review looks ok.

@tfoote
Copy link
Member

tfoote commented May 17, 2013

+1

@trainman419
Copy link
Contributor

I think $(find-share-devel ) and $(find-share-source ) are too complicated and should be left out. Do any of our users actually care about the distinction, or will it just clutter up the code and the documentation?

Does $(find-share PKG PATH) look in install space as well?

@tkruse
Copy link
Member

tkruse commented May 21, 2013

So, will $(find PKG) path be deprecated and raise a warning?
Will there be a REP to allow the community to vote, explaining the problem and alternatives? Rather than assuming that there is no better solution because none was found so far, it is actually possible to ask the community whether someone can think of a better solution. We do have some smart people in the community, I believe.

Does this mean that find-executable will find non-executable files as well?
If so, the name is bad. Documentation does not justify bad naming. And it does not look in the bin folder (i.e. what happens to my launch files if a package that I depend on moves one of its executables to the global bin folder)?
if dirname(PATH) is in some cases never evaluated, then it should be an optional param, e.g.
$(find-executable PKG FILE [SUBDIR]). If not given, source would not be searched.

Also the naming "share" requires users to understand the FHS layout, which makes it unnecessarily difficult for users. It is also not well-distinguished from "executable".

Unless I am mistaken, the general problem here is that the resource to be located is now not relative to a package-root, but to be found anywhere, really, where CMakeLists.txt decides to put the resource, and that we also need to suport the develspace, which has no definition.

The whole $(find ...) convenience was one of the many benefits of rosbuild avoiding FHS, allowing to share files accross packages as part of the public API of a package. Since with catkin we do not have the convenience of a package root folder in install layouts, we should avoid the generic $(find ...) syntax altogether and avoid "package-local paths" as an API betweeb packages altogether, unless something was explicitly provided by FHS and develspace tooling (E.g. something like pkg-config with custom keys and a develspace equivalent).

An alternative would be a new abstraction to package-resources, e.g. a strict mapping from source layout to FHS layout, e.g.

workspace/src/pkg/public_scripts maps to fhs_root/lib/pkg/public
and
workspace/src/pkg/public_resources maps to fhs_root/share/pkg/public

and we use find_executable / find_resource to look into those locations, and only those. If so, catkin could even by default install those public folders to the respective location as a cherry on top. This would make the API aspect of files explicit.

@dirk-thomas
Copy link
Member Author

@trainman419
Having the two commands find-share-devel and find-share-source is necessary since there are use cases where you need that functionality. They might not be used by a lot of people but still they are necessary.

Yes, find-share does also look in install space.

@tkruse
Deprecating find would be good. I am not sure I printing a warning for each time it is used is a good idea since this will currently be a lot.

This is more an API review than a REP. If people have suggestions about alternative ways of doing/naming things they are very welcome to comment during this review process.

As the name suggests find-executable will only find executables - no non-executables. So the name should be fitting. If another package moves its executable to the global bin than that is a change in the API. Every other package should just call the global binary than instead of using find-executable. Usually that should be done in a tick-tock cycle.

If you think share is not a good naming please propose an alternative.
If you propose to remove all find-like commands what would be the alternative proposal how to express the same use cases?

Could you detail your alternative with find_resource? What arguments does it take? How does it fulfill all the use cases of the above introduced commands? I suppose it will only work to identify existing files. E.g. using it to point to a specific path (which could be used by a program to dump stuff into) would not be possible.

@tkruse
Copy link
Member

tkruse commented May 22, 2013

Sorry, i expressed myself badly. We want to keep launchfiles, and thus we still need to refer to files provided by other packages, so we do need some find mechanism. What I wanted to say was that with catkin we need to drop "package-local" paths as the key to find any such file. Instead, a different clean and standard approach for using files as an API should be used with resources in catkin packages. So we'd still need some $(find ...) syntax, but whatever comes in the brackets should not be a package local path, as with catkin there is no definition of a package root folder that holds for both source and install spaces. Rosbuild allowed to access any source file within any other package. While this has not been overly abused, it was a systematic weakness that package internal files were thus part of the public API. So instead I suggest that package contents are generally protected from being "seen" by other packages, unless explicitly declared to be public. so catkin should allow packages to declare what files are public both for the devel and the install space, and the $(find ...) mechanism should only allow retriving those files.

That is the requirement. For how it is implemented there are many different strategies. Just as examples: One is to keep it like rosbuild and make the whole package visible, and to force the $(find ...) mechanism to explicitly make the mapping between devel and install space, e.g.
$(find install/location devel/location).

This is very cumbersome, so one alternative is what you suggested, several find-... calls with some magic behind the scenes that even if documented will confuse the users.

Both alternatives still allow users to use files from other packages that they should not use (because those files may move at any time).

So what I suggest is to have an 1:1 mapping from folder in source space to folder in install space, that has no complex functions behind the scenes, and that allows to define a public file-resource API of a package. However many other alternatives are also thinkable. E.g. there could be a catkin macro that declares "public" folders, like

catkin_public_folder(scripts ${CATKIN_PACKAGE_PUBLIC_DESTINATION}/scripts)

Where this macro would be responsible to create some artifact in the develspace that would allo a $(find ...) command to find files trasparently in install or source layouts. Or this could be implicit via named folders, just as per convention the "include" folder is where public headers go to. My preference would be for "public" subfolders in both the source space and the FHS layout as this is what novice users will most easily understand, I believe, and I personally would prefer this to work without manual invocation of an additional cmake macro. But "export" or a similar name would work as well.

I am not aware of any Unix/FHS standard for sharing files between packages other than e.g. c header files. or python modules, but maybe somebody else does. I believe /share in FHS does not imply "sharing" files between packages, and so we should avoid abusing that verb for that purpose, IMO. "share" should remain an FHS term that has nothing to do with how packages communicate with each other via files.

@dirk-thomas
Copy link
Member Author

A convention for how to layout a package and where different parts will automatically be deployed is a great idea and we will definitely consider that for the new development (aka ROS 2.0). But applying this to the existing packages is unrealistic. So we need a way to deal with the current diversity how packages look like.

Since the layout in the package source is arbitrary the suggested commands specify the paths as arguments. For scripts the install location is defined and therefore does not need to be specified. For other resources (which go into share) they are required to live in the same relative location which is a pretty straight forward convention. Naming these two command find-executable and find-share than seems to be a natural choice.

@jack-oquin
Copy link
Member

I agree with Thibault that a REP is needed for such a pervasive change. It will affect everybody.

I currently have a problem resolving the file name of a downloaded test file, which gets stored in the build/ subtree. As best I can tell, none of these proposals help solve that.

@tkruse
Copy link
Member

tkruse commented May 23, 2013

@dirk-thomas

As long as there is no realistic roadmap for ROS2.0, I won't believe it will ever happen, and I am not satisfied with any "We'll do it dirty for now, but in ROS2.0 we will clean things up" phrasing. That's just a lame excuse for accumulating technical debt never to be repaid. every change we make now should be an improvement, there are not enough resources to introduce something now that we don#t like and want to change again in the future.

Regarding the "naturalness" of the names, a new ROS developer might never run make install and might never have heard of FHS, nor care. The name "find-share" then tells such a person nothing at all. The name seemsnatural to you because you know FHS. So I would rather see find-file, find-resource, find-path or something.

Will the find-file also resolve subdirectories? e.g.:

 $(find-share 2dnav_pr2 /move_base)/navfn_params.yaml

Also such a person may use find-share in devel space between packages forgetting to install the resource. For this person this will never fail, for other people installing this will fail. This is systematically bad, as it causes broken packages to be distributed and released. It would be better if find-share relied on something in the develspace, not the source space, and a macro that makes this fail when resources are not being marked for install.

Also I was just thinking of rosrun, which AFAIK crawls for executables also in the source space. Wouldn't it be better to specify:

$(find-executable package filename)

thus that it does exactly what rosrun does, instead of the basename(PATH) magic you suggested?

@dirk-thomas
Copy link
Member Author

Based on the feedback I have removed $(find-share-devel) and $(find-share-source) from the patch. They can be readded if there is explicit need for it.

I also renamed $(find-share) to $(find-resource). The alternative names (find-file and find-path) seem to be less appropriate than find-resource since the command is able to find both files and folders.

The new find- command will resolve to a path (or error out) based on the arguments passed in. Anything behind the parenthesis is not considered. In example $(find-share 2dnav_pr2 /move_base)/navfn_params.yaml the content of the command is resolved to /some/path/to/2dnav_pr2/move/base. The text behind the command stays just as it is.

Regarding the fact that stuff might work in devel space but not in install space if the developer forgets to install certain files. That is true in any scenario and not related to roslaunch or its commands at all. E.g. if a package builds a binary it is runnable in devel space but does not work in install space if the executable is not installed.

The reason why we do not perform file system crawling: rosrun is able to resolve conflicts interactively which is not an option for roslaunch, it is much more expensive, and resolving resource by specific paths is common practice in existing tools (e.g. pkg_resources).

@dirk-thomas
Copy link
Member Author

Regarding the suggestion to start a REP for this:
a REP should specify common interfaces (like semantic messages, tf names) or structural standards (like the FHS layout, catkin workspaces). If you think the resolve strategy in an FHS compliant workspace needs more clarification I would suggest to propose an extension to one of the existing REPs (122, 128?).

The currently existing command $(find) does not fit the demands of FHS layouts. The result of the command depends on the paths following the command which makes it context dependent. For backward-compatibility we have no other choice than to make this behavior work since currently a lot of use cases are broken because of that. So this part of the patch is a bug fix.

$(find-executable) and $(find-resource) are the commands which can replace the bad API of $(find) by splitting the functionality and providing a cleaner API (by making the path an actual argument). If you think we need more functionality than to resolve executables or resources or want to propose a different resolve strategy please describe them and we will gladly consider them.

@wjwwood
Copy link
Member

wjwwood commented May 23, 2013

+1 to merging the $(find) function enhancements as a bug fix.

+1 to adding $(find-executable) and $(find-resource) as an alternative to $(find) now, even if we lack a full consensus. I personally think these functions are sufficient and the correct way to implement this resource discovery. I would be, however, interested to see more discussion on this as part of a discussion on ros-users, or as a proposed change to an existing REP, like the FHS REP for example.

+0 on dropping $(find-share-source) and $(find-share-devel), as dropping these keep the user from being explicit when they want to be, but I can understand how it can complicate or confuse the API.

-1 to making a stand alone REP for this. I do not think it is sufficient to justify its own REP, but I would entertain one if someone wanted to write it up.

@jack-oquin
Copy link
Member

Will $(find-executable) resolve C++ or Python executable files wherever they may be found? But, not Python modules?

Will $(find-resource) resolve files or directories containing CMake files, launch files, parameter files, etc.? If installed they come from PREFIX/share/PKG? Otherwise they come from either devel-space or the src tree, overlaying the install space?

Is there any way to resolve library names? Python modules? Header files? Is there any need for them?

How are test files resolved? I am having trouble with download_test_data() because it stores resources in the build tree, which roslaunch and rostest cannot resolve. This proposal does not help with that, does it?

@wjwwood
Copy link
Member

wjwwood commented May 23, 2013

Will $(find-executable) resolve C++ or Python executable files wherever they may be found?

From @dirk-thomas's original post $(find-executable PKG PATH) will look first in <prefix>/lib/share/<pkg_name>/basename(PATH) and then fall back to SOURCE_SPACE/PATH. So no it will not crawl or search for executables for the reasons mentioned above.

But, not Python modules?

No, python modules should not be marked executable, why would find-executable locate those?

Will $(find-resource) resolve files or directories containing CMake files, launch files, parameter files, etc.?

The type of resource does not matter.

If installed they come from PREFIX/share/PKG? Otherwise they come from either devel-space or the src tree, overlaying the install space?

Yes.

Is there any way to resolve library names?

pkg-config

Python modules?

PYTHONPATH

Header files?

pkg-config for the header include paths, gcc can find them from there.

Is there any need for them?

Not that I can see.

How are test files resolved?

Like any other resource.

I am having trouble with download_test_data() because it stores resources in the build tree, which roslaunch and rostest cannot resolve. This proposal does not help with that, does it?

This proposal doesn't cover discovering things in build space, no. I would say, having not dug into that problem completely, that you can either move the resource into the devel space for testing or you can template your test files with the build space location of the resources before running. Currently we cannot run tests on installed packages so if devel space mimics install space then the test resources should not go in either. They also do not belong in the source space, so I would say that leaves the build space. I think the templating of the test files to contain the absolute path of the test resources or running the tests in the build space and using relative build space paths in the test files are the best options.

@jack-oquin
Copy link
Member

We will never be able to explain most of this stuff to other robotics developers.

-1 to $(find-executable) not being able to find executables in a natural way. I don't understand the rationale for why it requires a hack, and neither will most of our users.

+1 to $(find-resource) being able to find most commonly-needed files and directories.

-10 to $(find-resource) not being able to find downloaded test data.

It looks like putting downloaded files in build space was just a hack, due to a lack of better ideas. No wonder the download_test_data() command was never documented: too embarrassing to admit what it does.

Since putting the files in devel space apparently would work, I suppose I favor that option. Maybe someday we will create install-able test packages, but I'm not holding my breath waiting for that to happen.

So, it seems that the download problem is mainly the result of poor choices in the download_test_data() implementation. Hopefully that can be resolved via ros/catkin#426.

@dirk-thomas
Copy link
Member Author

@jack-oquin

Can you please describe why $(find-executable) is "not being able to find executable in a natural way" and why you consider it a hack?

Regarding the test data we have the issue ros/catkin#426 to track that (with the already proposed workarounds in this thread). This will not effect the roslaunch commands since handling of these test data can and should be done in a roslaunch agnostic way.

@jack-oquin
Copy link
Member

I agree with @tkruse that $(find-executable) should work like rosrun. That is what users will expect, without even thinking about it.

The explanation seems hacky and unnatural to me: "The reason why we do not perform file system crawling: rosrun is able to resolve conflicts interactively which is not an option for roslaunch, it is much more expensive, and resolving resource by specific paths is common practice in existing tools (e.g. pkg_resources)."

No one but @dirk-thomas and a few of his friends are ever going to understand that. :-)

@dirk-thomas
Copy link
Member Author

If I would have to choose between:

(1) create a convention how a package should be structured, place all scripts into a subfolder called foo and than let rosrun only execute the scripts in this folder

(2) let the developer place their scripts in a random location, when rosrun is invoked crawl the file system recursively for files which have the same name and are executable

I will always pick (1). Why? Just because of:

  • no ambiguity
  • more efficient

Do I think I am "alone" with this opinion? I am pretty sure numerous of existing tools are following this concept. pkg_resource is a good example of that - it does not perform recursive crawling to find the resources. Even until now roslaunch does not support any kind of rosrun-like command to find executables.

@wjwwood
Copy link
Member

wjwwood commented May 23, 2013

@jack-oquin, to be fair, I don't see how these points are 'hacky' or unnatural:

PKG_NAME and EXEC_NAME are not sufficient to identify a single executable in the source space when searching recursively

This is not a hack nor is it unnatural in any way I can see.

Crawling is more expensive

This is a fact, if some one has a very deep test folder, for example, crawling this would be more expensive, this is not a reason out right to not behave like rosrun, but we should strive to avoid it. I think it is a safe goal to prefer configuration over searching.

Again not hacky or unnatural.

Resolving resources by specific paths is used by other systems, e.g. Python's pkg_resources

pkg_resources solves this exact problem, but they do it by making you manually specify the "resources" that others should be able to find explicitly. All of the CMake projects I have seen have used similar strategies for resource resolution, in fact I don't know of another system which behaves like rosbuild.

To the point of it being unnatural, I have to disagree. Look at any UNIX path based system, you always specify a list of possible prefixes and an explicit postfix. For example:

  • gcc: you give it a postfix, e.g. boost/thread.hpp and it tries that postfix against all of its prefixes in CPATH
  • pkg_resources: you give it a relative path for a resource, e.g. templates/rules.em and it tries to find that against the resources declared in the python package
  • even the global PATH: you give it an exact postfix and it tries to match that against the prefixes in the PATH variable until it finds it

In this respect the $(find-executable PKG PATH) is very natural.

The only inconsistency I see is that it looks for the basename of the PATH you give it when searching the install/devel space, e.g. the PATH scripts/foo would be searched in this order:

  • <install>/lib/<package name>/foo
  • <devel>/lib/<package name>/foo
  • <source>/scripts/foo

This is odd when you first look at it, but this is not something we can change as the FHS does not allow us to put nested folders under the libexec equivalent folder.

"Applications may use a single subdirectory under /usr/lib. If an application uses a subdirectory, all architecture-dependent data exclusively used by the application must be placed within that subdirectory."
http://www.pathname.com/fhs/pub/fhs-2.3.html#USRLIBLIBRARIESFORPROGRAMMINGANDPA

We could make this more conventional if we said that there was only one folder searched in the source space, such that $(find-executable PKG EXEC_NAME) is resolved in this order:

  • <install>/lib/<pkg_name>/<exec_name>
  • <devel>/lib/<pkg_name>/<exec_name>
  • <source>/bin/<exec_name>

@tkruse pointed out something similar, "a strict mapping from source space to devel/install space" for resources, in his first post.

Either way searching recursively is not a good option in my opinion.

@jack-oquin
Copy link
Member

Perhaps I did not ask the right question, because I certainly did not understand the answers I got.

Will $(find-executable) find exactly the executables which would be found via a <node type=executable .../> element in a launch script?

@dirk-thomas
Copy link
Member Author

$(find-executable PKGNAME PATH/OF/EXECUTABLE) will resolve to specific executable with can be either in devel space (under lib/PKGNAME/EXECUTABLE) or source space (PKG_FOLDER/PATH?OF?EXECUTABLE).

The node invocation would return the same result if the file named EXECUTABLE does only exist once in the package source space.

@jack-oquin
Copy link
Member

For me that answer requires further simplification: was that "yes, if only one source file has that executable name"?

@tkruse
Copy link
Member

tkruse commented May 23, 2013

@dirk-thomas: calls to $(find... ) are an API, one that affects all users of ROS, so I believe that justifies a REP. The size of the problem or the solution have no relevance as to what should be a REP or not. A REP is a signal to the community that OSRF is interacting rather than dictating, the value of a REP thus is also symbolic beyond the mere technical value.
We have no other choice than to fix it, but the REP would not be about whether to fix it or not, but about how to fix it.

I am also in favor of using fixed folders for executables tather than requiring a user to type it in the roslaunch command. E.g. if a package has a script foo in package/scripts/foo, and another package depends on it in a launch file, and the script moves to package/nodes/foo, then i dont want the dependent script to break because it specified "scripts/foo". For executables, what one package should ever know about another package is "I want to run the script foo that is provided by package X". Knowing in which subfolder structure relative to the package root that script resides should not be relevant to a dependent package. This would also make roslaunch files future-proof, for whatever layout future generations of ROS may use.

Else, at least the syntax of the command should be like this:

$(find-executable package name [source-path-disambiguation-hint]) so that the API shows what happens behind the scenes.

@wjwwood: "Applications may use a single subdirectory under /usr/lib." I am not sure whether this means no further nesting is allowed, or whether it merely means no other siblings are allowed. E.g. on Ubuntu I see:

/usr/lib/gimp/2.0/environ/default.env
/usr/lib/nautilus/extensions-3.0/libnautilus-fileroller.so
/usr/lib/python2.7/importlib/__init__.py
etc.

They might all be abusing FHS, I don't know.

Crawling inefficiency can be mitigated by useful heuristic and conventions. If the first valid result is always chosen, then the realistic average case can be found in O(1) with conventions, even if the theoretical case were O(n).

@wjwwood
Copy link
Member

wjwwood commented May 24, 2013

E.g. if a package has a script foo in package/scripts/foo, and another package depends on it in a launch file, and the script moves to package/nodes/foo, then i dont want the dependent script to break because it specified "scripts/foo".

The location of the script is part of the interface, just like if I move a header file into a different folder but still on my include path, any dependent code will break.

This is more of a reason to do explicitly exported resources in my opinion, like pkg_resources.

If you interpret the FHS to allow sub folders in the lib/pkg-name folder then another option is to mirror the structure in devel/install and source space, such that $(find-executable foo scripts/bar) resolves as:

  • <install>/lib/foo/scripts/bar
  • <devel>/lib/foo/scripts/bar
  • <source>/scripts/bar

@jack-oquin
Copy link
Member

The FHS clearly states that packages may only use a single /usr/lib subdirectory. We discussed that in the REP-0122 review.

It would obviously be helpful in some cases to use a subtree, instead. That is probably why the packages Thibault mentions chose not to follow the standard. Of course, some of them may predate that standard and be "grandfathered" in.

In the review, we decided not to push it. I suppose that decision could be re-visited.

@jack-oquin
Copy link
Member

I agree that explicitly exported resources are an attractive notion.

I doubt I understand all the implications of that approach, however.

@tfoote
Copy link
Member

tfoote commented May 24, 2013

As a general comment on this thread please avoid descending into ad hominem arguments: https://en.wikipedia.org/wiki/Ad_hominem

@tkruse With respect to the use of REPs, they are one way to review and discuss design decisions. There are many other mechanisms by which community feedback can be received such as a change review discussed in a public forum and circulated on the appropriate mailing list. The size of a decision does effect what forum the decision should be discussed. Engaging a larger community group with a more formal process takes significantly more effort. If we spend a few days writing a REP for every patch we can not do much work. Likewise, if we write REPs for every small change the signal to noise ratio of reading the REPs goes down to the point that most people cannot find relevant information in the REPs (we've already gotten feedback that people can't find things which are specified in the REPs). If we were to decide to review the roslaunch xml API in it's entirety and consider how to improve it as a whole writing a REP for that migration process would make sense. However this change is required to complete the implementation for REP 122 and REP 128, we are trying to make the minimum change to support the previously discussed changes to specifications. Lets just continue with this forum. It's been circulated to the community, and some of our most active community members are here discussing it. We've made progress towards a cleaner API based on the ongoing review.

@jack-oquin
Copy link
Member

The only ad hominem argument I am intentionally making is directed at myself. The rest of you know far more about the internal logic of this stuff than I do. The most useful role I can play in this discussion is "dumb user". If I can't understand the logic of this design, there are many other ROS users who will also have difficulties.

That is why I am asking a lot of simple questions. When I don't get simple enough answers, there may be too much complexity. Or, maybe I'm just not smart enough to understand. You decide.

@tkruse
Copy link
Member

tkruse commented May 24, 2013

@tfoote:

If we spend a few days writing a REP for every patch we can not do much work. Likewise, if we write REPs for every small change the signal to noise ratio of reading the REPs goes down to the point that most people cannot find relevant information in the REPs...

Nobody said anything about "every patch" or "every small change", so this is an exagerration and a strawman anyway. If you say this issue is not worth any REP action we'll just have to agree to disagree.

A lot of real work goes into a REP, not just wasting time, but thinking. At least if the REP gives a quality rationale, and is not a mere expanded commit message. Even using the term "signal to noise ratio" denigrates the good efforts of REP authors.

If you can integrate the change into REP122 or REP128 that's also fine by me, let the community vote on a REP delta.

Lets just continue with this forum. It's been circulated to the community, and some of our most active
community members are here discussing it.

A github pull request is not a reliable archive for any open-source project, I am already a bit ashamed of writing here and not in the buildsystem SIG. As far as the community is concerned, this here is a hidden discussion.
Spreading discussions over different sites and media does not help ROS.

The presence of the most-active community members is not sufficient, the presence of the random lurker who only once per decade contributes a great idea is still valuable to any discussion, and one of the benefits of doing things open-source.

@jack-oquin:
Even if the nesting ob subfolder in /usr/lib had already been discussed, I won't be convinced that discussion found the right interpretation unless a good source or rationale is given. The only discussion I found is here: https://groups.google.com/forum/#!topic/ros-sig-buildsystem/8eSMQyAJo0o
And it does not look to me as if anyone had questioned the interpretation of that FHS sentence there, so it was not really a discussion to my eyes, but merely a prematurely accepted opinion.

Also I believe the FHS rules were not made arbitrarily to make life hard for packagers, but each decision had a rationale. Allowing only one directory without siblings is useful to keep the namespace in /usr/lib small. Forbidding further nesting of subfolders has no technical or other benefit I could see. I don't even find a single other open-source project where nested subfolders would be discussed as problematic.

@jack-oquin
Copy link
Member

@tkruse:

The main topic of that discussion was how to fix the rather more serious problem caused by Fuerte shipping machine-dependent binaries in subdirectories of share/.

We did not spend much time discussing lib/PACKAGE/ subdirectories, because the standard was quite clear on that subject. There didn't seem to be much more to say, we could either follow it or choose to differ. No one provided a clear reason not to comply, so we did. As I said earlier, if we come up with a compelling reason not to follow the FHS in this area, we can re-open the discussion.

I don't know why the FHS decided to restrict subdirectories, but their wording seems unambiguous to me:

Applications may use a single subdirectory under /usr/lib. If an application uses a subdirectory, all
architecture-dependent data exclusively used by the application must be placed within that subdirectory.

What other interpretation do you perceive? Does "within" mean "in or below" to you?

@jack-oquin
Copy link
Member

@tfoote:

I concur with your judgement that this discussion does not require a separate REP. And, if we decide to amend REP-0122 or REP-0128 to document this discussion, I am willing to help with writing or reviewing.

However, I think @tkruse has a point: github issues are not a good forum for public discussion on matters of general interest. The ros-sig-buildsystem mailing list is well-established and competent to review these decisions. People wishing to understand our thinking in this subject will likely be able to find those messages many years from now.

For a project like ROS, the perception of openness is perhaps as important as the quality of our choices. So, I would vote for opening a thread on ros-sig-buildsystem. We can link to this issue so others can catch up without needless repetition.

@tkruse
Copy link
Member

tkruse commented May 24, 2013

@jack-oquin
Yes, "within" to me means "anywhere below", as opposed to putting some files directly into /usr/lib, and some files under /usr/lib/foo.

@jack-oquin
Copy link
Member

@tkruse:

Then, I can understand why you think it's debatable.

For my dialect, there is only one reasonable interpretation that accords with everyday English usage. "In" and "below" are not synonyms.

Others may differ, of course. Could your interpretation be influenced by other languages, perhaps?

@wjwwood
Copy link
Member

wjwwood commented May 24, 2013

I have to disagree about a github pull-request being a bad forum for open discussion, it is open to everyone, searchable, highly scoped, and has an integrated timeline with the changes to the proposal. In fact I think it is the ideal place to discuss a change to the API of a package. In my opinion, in the far future people will not be crawling through the build system sig mailing list looking for memoranda, first they will be searching the commit history and related pull request issues.

When we decide to move to another code hosting service, and we will at somepoint, the issues/pull-requests are all downloadable with all of their history into a plain old json structure. I guarantee what ever the next big code hosting system is, it will easily import from github.

That being said I have no problem moving this discussion to the build-system-sig mailing list or even duplicating this conversation there.

@dirk-thomas
Copy link
Member Author

As a consequence of this lengthy discussion I have created pull request #233. That one only fixes the behavior of the $(find) command by considering the path following the command for the resolving. It will address the current needs to fix broken behavior. If you have comment on this part only please consider to comment on that.

The new commands $(find-executable) and $(find-resource) (which would be necessary to fix the recent xacro issue cleanly without copying the executable into multiple locations) will be left unimplemented until someone will write a REP (extension) and an consensus is reached on that.

@jbohren
Copy link
Member

jbohren commented May 24, 2013

  1. This all looks terrifying.
  2. Is there any reason why this sort of change wasn't posted to the roslaunch sig?

@wjwwood
Copy link
Member

wjwwood commented May 24, 2013

It would be helpful if that SIG were listed on the list of active SIG's: http://www.ros.org/wiki/sig#Active_SIGs

I didn't even know it existed.

@jbohren
Copy link
Member

jbohren commented May 24, 2013

Yeah, it's listed on the list of active SIGs in Section 3.1.

@jbohren
Copy link
Member

jbohren commented May 24, 2013

Oh wow I didn't even know about the "active" section. Well, it's definitely been active in the last few months.

@jack-oquin
Copy link
Member

Sorry, Jon. I didn't know about that SIG either.

It certainly seems a good forum for this discussion.

@dirk-thomas
Copy link
Member Author

Closing this pull request due to lack of interest for the proposed functionality.

@dirk-thomas dirk-thomas deleted the roslaunch-find branch June 17, 2013 18:45
@jack-oquin
Copy link
Member

This seems like a rather long discussion to reflect "lack of interest".

Why do feel that interest is lacking?

@dirk-thomas
Copy link
Member Author

The already applied patch for $(find) resolves the current bugs. The two additional commands proposed here should provide a cleaner API (not considering the path which comes after the "function") but were rejected in the discussion.

Since I do not have any eminent use case for that I am not pushing on it. But since the last conversation nobody else seems to be interested to propose something different or to continue the discussion here or on the roslaunch mailing list. Therefore I closed the pull request without applying it.

Anybody feel free to continue the discussion on the mailing list if you have a use case for this.

@jack-oquin
Copy link
Member

I see.

The rostest problem I mentioned earlier has been very well addressed by your new catkin_download_test_data() command. So, I have no further immediate requirements, either.

I agree that the roslaunch SIG is a good forum for discussing any additional needs not already being met.

@mikepurvis
Copy link
Member

I know I'm super late to this party, but did the $(find-resource) functionality end up implemented some other way?

@dirk-thomas
Copy link
Member Author

No, $(find-resource) has not been implemented since this pull request has not been merged. Only the behavior of $(find) has been fixed to address the urgent needs problems of the original implementation. So you still have to use $(find) and its implicit heuristic to find the resource you want to refer to.

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

Successfully merging this pull request may close these issues.

8 participants