rospack find xacro no longer finds the directory containing xacro.py #2

Closed
jon-weisz opened this Issue Mar 5, 2013 · 27 comments

Comments

Projects
None yet
9 participants

Catkinizing xacro moved the install directory of xacro.py so that it is no longer in the directory found by 'rospack find xacro' . This breaks launch files that previously loaded the urdfs into the parameter server using <param name="robot_description" command="xacro.py '$(find robot_dir)/robot_description.xacro' />

Owner

tfoote commented Mar 6, 2013

We need to put a redirect script into the share directory to provide backwards compatibility for this.

Owner

tfoote commented Mar 6, 2013

@dirk-thomas Did you have an example of this?

Owner

dirk-thomas commented Mar 6, 2013

set(PYTHON_SCRIPT path-to-my-script.py)
configure_file(${catkin_EXTRAS_DIR}/templates/script.py.in
  ${CATKIN_DEVEL_PREFIX}/${CATKIN_PACKAGE_SHARE_DESTINATION}/my-script.py
  @ONLY)
Owner

dirk-thomas commented Mar 6, 2013

At best the script in share should also output a deprecation warning so that packages which use that file can update to the new usage. So probably it is best to create a another script in xacro which outputs the warning and than calls the same as the script in bin.

Owner

tfoote commented Mar 6, 2013

So I came up with the following. @dirk-thomas unless you think you can clean this up quickly (in particular the installing configured files). I think I'm going to take advantage of the rollback capability. There's a lot of corner cases we need to cover.

# Backwards compatability put xacro.py into share so $(find xacro)/xacro.py will work in launch files           
# devel version                                                                                                 
set(PYTHON_SCRIPT ${CATKIN_DEVEL_PREFIX}/${CATKIN_PACKAGE_BIN_DESTINATION}/xacro.py)
configure_file(${catkin_EXTRAS_DIR}/templates/script.py.in
  ${CATKIN_DEVEL_PREFIX}/${CATKIN_PACKAGE_SHARE_DESTINATION}/xacro.py
  @ONLY)

# backwards compatability for installed file                                                                    
set(PYTHON_SCRIPT ${CMAKE_INSTALL_PREFIX}/${CATKIN_PACKAGE_BIN_DESTINATION}/xacro.py)
configure_file(${catkin_EXTRAS_DIR}/templates/script.py.in
  ${CATKIN_DEVEL_PREFIX}/${CATKIN_PACKAGE_BIN_DESTINATION}/to_be_installed/xacro.py
  @ONLY)
#install the backwards compatability script too                                                                 
install(PROGRAMS ${CATKIN_DEVEL_PREFIX}/${CATKIN_PACKAGE_BIN_DESTINATION}/to_be_installed/xacro.py DESTINATION \
${CATKIN_PACKAGE_SHARE_DESTINATION})

It unfortunately leaves multiple xacro.py executables in the path for rosrun.

In install space:

tfoote@btn:/tmp/ws/src/build/install$ rosrun xacro xacro.py 
[rosrun] You have chosen a non-unique executable, please pick one of the following:
1) /tmp/ws/src/build/install/lib/xacro/xacro.py
2) /tmp/ws/src/build/install/share/xacro/xacro.py
#? 2

And more in devel space:

tfoote@btn:/tmp/ws/src/build/devel$ rosrun xacro xacro.py 1
[rosrun] You have chosen a non-unique executable, please pick one of the following:
1) /tmp/ws/src/build/devel/lib/xacro/to_be_installed/xacro.py
2) /tmp/ws/src/build/devel/share/xacro/xacro.py
3) /tmp/ws/src/xacro/./scripts/xacro.py
4) /tmp/ws/src/xacro/scripts/xacro.py

This creates the nasty temp file which should be avoided.

Also somehow xacro.py is being installed into the global bin dir as well. It needs to be taken out of the scripts in setup.py

catkin_find --share --first-only xacro/xacro.py doesn't work either in devel space.

Member

tkruse commented Mar 6, 2013

"At best the script in share should also output a deprecation warning so that packages which use that file can update to the new usage."

What is the new usage? What would users have to put into their launchfiles to find the scripts in lib?

In my opinion, the (find ...) shortcut for roslaunch files should be changed to find files the way rosrun does.

Owner

dirk-thomas commented Mar 6, 2013

@tkruse: how would you change the (find ...) logic? It only finds packages not files within the package so it has no chance to guess what to return - share or lib would be equally wrong depending on the case.

Member

tkruse commented Mar 6, 2013

Yes, the whole roslunch interpreting would have to be changed so that (find xyz)/foo is evaluated as a whole, not the brackets insolation.

Contributor

ablasdel commented Mar 7, 2013

The backwards compatibly xacro.py script clearly has to stay the same name. But it seems that it might be prudent to rename the version we are putting in the lib directory to cut down on these issues.

Owner

tfoote commented Mar 19, 2013

After a long discussion, the only way to keep backwards compatability is to install scripts into the places it's expected in the install space (share) and devel such that the $find will work in both devel and install space.

Note this is only really an issue for scripts. Resources should simply be installed into share. For scripts they will also need to end up in the correct libexec location for forward looking FHS compliance. And writing a new $(script packagename scriptname) macro is probably in order for roslauch to replecate the $(find package) logic.

Member

tkruse commented Mar 19, 2013

Note this is also an issue for package local binary executables, those may also be both rosrunned as well as roslaunch found. Users need a recommendation for how to handle that as well.

Also, I suggest that whereever a script is duplicated that way, as soon as ros/ros_comm#199 is fixed, the script's alter ego in share should be deprecated, meaning it should warn the uer and recommend using the executable in lib using. So maybe we can have a catkin macro that creates the alter-ego script in share, such that at some point we can change this macro to create an alter ego that emmits the deprecation warning.

Contributor

ablasdel commented Mar 27, 2013

@dirk-thomas @tfoote
Was discussing this today. What are your thoughts on a solution as such:

xacro.py is renamed to "xacro" and is installed in the following way:
install(PROGRAMS scripts/xacro DESTINATION ${CATKIN_PACKAGE_BIN_DESTINATION})

A backwards compatibility script named "xacro.py" will live in /share and simply print a deprecation warning and run the "new" xacro script.

Does anyone see any problems with this method fully catkinize xacro?

Member

tkruse commented Mar 27, 2013

I guess that would mean "rosrun xarco xacro" would run on an installation, but not in the develspace.

Contributor

ablasdel commented Mar 27, 2013

In the devel space "xacro" it would exist in the scripts directory just
like any other script and should be rosrunable.

Contributor

ablasdel commented Mar 27, 2013

Maybe I wasn't being clear.
The "new" xacro script is the old xacro.py renamed and lives in the
standard catkin location.
The "new" xacro.py just invokes xacro with a dep warning and lives in the
share directory.

I've modified the cmake code from @tfoote to try and reflect what @ablasdel is suggesting. Additionally, i've renamed the backwards compatibility script in the devel space so that rosrun only finds one xacro.py in devel space. For this to work, i've renamed the xacro.py in source space to xacro. The result seems to be:

In source/devel space:
-original script called "xacro" in the source space
-backwards compatibility script "xacro.py" in the devel/share space, points to the xacro script in source space

In install space:
-installed script called "xacro" in lib/xacro
-backwards compatibility script "xacro.py" in share/xacro/

install(PROGRAMS scripts/xacro DESTINATION ${CATKIN_PACKAGE_BIN_DESTINATION})

# Backwards compatability put xacro.py into share so $(find xacro)/xacro.py will work in launch files
# devel version
set(PYTHON_SCRIPT ${xacro_SOURCE_DIR}/scripts/xacro)
configure_file(${catkin_EXTRAS_DIR}/templates/script.py.in
  ${CATKIN_DEVEL_PREFIX}/${CATKIN_PACKAGE_SHARE_DESTINATION}/xacro.py
  @ONLY)

# backwards compatability for installed file
set(PYTHON_SCRIPT ${CMAKE_INSTALL_PREFIX}/${CATKIN_PACKAGE_BIN_DESTINATION}/xacro)
configure_file(${catkin_EXTRAS_DIR}/templates/script.py.in
  ${CATKIN_DEVEL_PREFIX}/${CATKIN_PACKAGE_BIN_DESTINATION}/to_be_installed/backwards_compatible_xacro.py
  @ONLY)

# install the backwards compatability script too
install(PROGRAMS ${CATKIN_DEVEL_PREFIX}/${CATKIN_PACKAGE_BIN_DESTINATION}/to_be_installed/backwards_compatible_xacro.py DESTINATION
  ${CATKIN_PACKAGE_SHARE_DESTINATION} RENAME xacro.py)

You can try this change by pulling from the backwards_compat branch of my fork: https://github.com/jonbinney/xacro/tree/backwards_compat
(this doesn't yet add the deprecation warning though)

What's the progress on getting this released into Hydro? It's one of the things blocking the catkinization of the PR2 packages for Hydro.

After trying a bunch of things, the best fix I've found is to actually have 2 identical scripts.

  • "xacro.py" - lives in the root of the source directory for the package. Gets installed into the share directory for the package. Using $(find xacro)/xacro.py in a launchfile finds this whether working from source or an install
  • "xacro" - lives in the scripts directory in source. Gets installed to the lib directory for the package. Using "rosrun xacro xacro" finds this one in both source and install.

The reason for having two differently named scripts is that we need one with the old name, which can be found by the $(find) syntax in roslaunch files. But we should eventually transition to one which gets installed to lib/xacro since that is the standard fhs place for it.

Eventually we should have xacro.py print out a deprecation warning telling people to use the new "xacro" script in lib/xacro, but I don't feel comfortable doing that until there is a solution to using $(find) with scripts in roslaunch xml files.

I've created a hydro-devel branch in this repo with this two script solution. It also incorporates the subsitution arg patch from @jbohren . If the two-script setup sounds good to people, I can try releasing this into hydro.

+1 ; sounds good to me.

I've added a fix so that the groovy-devel branch should now be compatible with the $(find) syntax in launchfiles. This is done by having two copies of the xacro script, as described above. Probably not going to release in groovy, but the same fixes have been applied to the hydro-devel branch and released.

This has been sorted out, so I'm closing this issue. The end result is:

  • In fuerte and groovy, xacro will continue to use rosbuild
  • In hydro, we have a workaround that involves having two scripts
  • Dirk has come up with roslaunch syntax for using $(find) catkinized packages; once enough people have adopted this, we'll release xacro with the backwards compatibility script. (might be I-turtle, might be later)

@jonbinney jonbinney closed this Jun 2, 2013

Owner

dirk-thomas commented Jun 2, 2013

I am not sure if you want to reopen the issue but the currently applied changes to roslaunch (ros/ros_comm#233) will only allow a very restricted solution. The executable script must be in the root of a package to be found there and after installation under lib/PKGNAME since the find command does only specify one path of the script.

For the additional commands proposed in ros/ros_comm#228 which would enable an arbitrary location of the executable in the source package it was not possible to reach a consensus so it will likely not be merged at all.

Member

tkruse commented Jun 2, 2013

Isn't it possible to use a e.g. setup.py to install a relay script in the develspace that would be found?

Owner

dirk-thomas commented Jun 2, 2013

Sure, but that is just another workaround because we lack the necessary roslaunch logic from ros/ros_comm#228.

Starting in hydro we use a workaround that installs a script to share so that it cane be found by $(find). This works, but isn't ideal. I'll reopen this so that we remember to update things if the roslaunch syntax problem is resolved at some point.

@jonbinney jonbinney reopened this Jun 2, 2013

Owner

wjwwood commented Feb 3, 2014

I believe the relay scripts are all in place now for the backwards compatibility of roslaunch usages so I am going to close it. Please comment here if this is still not fixed.

@wjwwood wjwwood closed this Feb 3, 2014

nim65s pushed a commit to nim65s/robotpkg-wip that referenced this issue Nov 2, 2017

[wip/dynamic-graph-bridge] patch install to be able to use ros facili…
…ties

patch-ac
--------
install ros executables also in share since
ros facitilies are not able to find it in bin
for now
this issue has already been raised by ros community
(e.g.  ros/xacro#2)
should be possible to remove the patch with Hydro version (need to be tested)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment