Only perform backquote substitution when needed #34

po1 · 2014-02-15T14:11:54Z

This pull request has several goals.

fix an issue when using pluginlib in a Xenomai environment, for example through ros_control. In such an environment, calling popen can have disastrous consequences. While not preventing the call, the proposed fix removes unnecessary calls.
Improve speed of pluginlib. A call to createInstance() has been benchmarked to drop from around 2 seconds to 50 milliseconds with this fix on our local setup with a near-complete ROS hydro install + extra local packages.
Improve compatibility with non-POSIX systems, which have no bourne-compatible shell and thus cannot perform the substitution. This point should be further addressed by redesigning the substitution to match that of other components, e.g. launch files.

It is proposed against groovy-devel because it is very small and does not break API or ABI compatibility.

dirk-thomas · 2014-02-15T18:09:23Z

src/rospack.cpp

@@ -2151,7 +2151,12 @@ bool cmpDirectoryCrawlRecord(DirectoryCrawlRecord* i,
 outstring.replace(i, std::string(MANIFEST_PREFIX).length(), 
 stackage->path_);
 }
-
+
+ if (outstring.find_first_of("$`") == std::string::npos)


Why is it searching for $`` and not only for ```? And if it should search for backtick only it might better use .find()`.

I don't know exactly what the intention was, but you can use the alternative syntax $(command)
Because I didn't know exactly who is using this feature, I just preferred not taking chances.

Wouldn't it be better to search for ``` as well as $(?

This is what find_first_of is doing. It is searching for ``` and $

Ow, sorry, I think I got what you meant. You mean that we should look specifically for $( and not just $, right?
Well, that would also break the previous behavior. I don't know if anybody actually relies on this, but again, not taking chances.

So it should process:

commands wrapped in backticks (...)

commands wrapped in $(...)

environment variables accessed with $name

Then the patch makes sense as it is.

To clarify that in the patch can you please insert the following comment above the if line:

// skip substitution attempt when the string neither contains // a dollar sign for $(command) and $envvar nor // a backtick wrapping a command

po1 · 2014-02-15T19:01:28Z

@dirk-thomas the comment has been added.
Sorry for the extra commit, my editor gets crazy (and so do I) with trailing spaces.

dirk-thomas · 2014-02-15T19:04:22Z

As long as it gets applied to all active branch and is in a separate commit I am fine with code style clean up. Can you please squash the first two commit into one?

Spawning shells is an expensive operation. In a lot of cases, there is no need for any substitution. Besides, this does not work on Windows.

po1 · 2014-02-15T19:16:14Z

Here you go

dirk-thomas · 2014-02-15T19:18:25Z

+1 form me.

@tfoote @wjwwood Please review and consider that it will be applied to Groovy, Hydro and Indigo.

tfoote · 2014-02-16T09:25:11Z

+1 with backport . The improvements are backwards compatible and provide support for more platforms. If we're backporting to hydro we might as well go to groovy too.

dirk-thomas · 2014-02-17T01:38:06Z

Thank you for the patch.

I have applied your changes to the indigo-devel branch (27ed245, ac74cfe). I will merge it to Groovy and Hydro once I am going to do new releases for them.

wjwwood · 2014-02-17T01:49:01Z

+1

adolfo-rt · 2014-02-17T11:12:20Z

+1, yay!

Improve compatibility with non-POSIX systems, which have no bourne-compatible shell and thus cannot perform the substitution.

This seems like legacy from the rosbuild era. As an indication, a search over all 418 package.xml files in my current system yields that:

$ is only used in the context of ${prefix}
Backticks are only used in non-comments by three packages: map_server, laser_filters and diagnostic_aggregator for rosboost-cfg --cflags or rosboost-config --lflags. Setting these flags in the CMakeLists.txt should bring the count to zero.

This point should be further addressed by redesigning the substitution to match that of other components, e.g. launch files.

+1 for $(find package_name) syntax support, if there are usecases for it. The call to popen will have to remain there until rosbuild is phased out, though.

I have applied your changes to the indigo-devel branch (27ed245, ac74cfe). I will merge it to Groovy and Hydro once I am going to do new releases for them.

Nitpick: Please cherry-pick the changesets next time, so attribution is preserved ;-).

dirk-thomas · 2014-02-17T17:46:53Z

@adolfo-rt Despite what is actively being used in a very small subset of packages we can simply not change behavior. What has worked before and was valid code should continue to work.

All patch always go to the latest development branch and then get backported when they have proven stable. But these changes did not at all apply to indigo-devel - please try a cherry-pick - both branches have divered significantly. Therefore I just replicated the patch instead of waiting for the pull request to be modified to apply to indigo-devel. Usually I will always either cherry-pick or merge the incoming if they are good as they are. Especially the trailing white space part was also extended to cover all sources but a single file.

adolfo-rt · 2014-02-18T08:31:24Z

@adolfo-rt Despite what is actively being used in a very small subset of packages we can simply not change behavior. What has worked before and was valid code should continue to work.

I'm not suggesting breaking existing behavior in the short or medium term. Still, if platform-independence becomes important enough at some point to be addressed, this would definitely imply changing behavior. That being said, I find it a good sign that most catkin packages out there are not relying on platform-dependent behavior :).

dirk-thomas · 2014-02-25T22:35:42Z

Cherry-picked the change (without the whitespaces) into groovy-devel. Will be released as 2.1.23.

adolfo-rt · 2014-02-26T09:51:47Z

Thanks!

Drop patch-aa for tinyxml location, this is now handled by devel/ros-cmake-modules. Changes since 2.1.19: 2.2.4 (2014-07-10) ------------------ * fix find_package(PythonLibs ...) with CMake 3 (`#42 <https://github.com/ros/rospack/issues/42>`_) 2.2.3 (2014-05-07) ------------------ * find library for exact Python version (even if not in CMake provided list of version numbers) (`#40 <https://github.com/ros/rospack/issues/40>`_) * find TinyXML using cmake_modules (`#24 <https://github.com/ros/rospack/issues/24>`_) * make error messages tool specific (rospack vs. rosstack) (`#38 <https://github.com/ros/rospack/issues/38>`_) 2.2.2 (2014-02-25) ------------------ * python 3 compatibility (`#35 <https://github.com/ros/rospack/issues/35>`_) 2.2.1 (2014-02-24) ------------------ * only perform backquote substitution when needed (`#34 <https://github.com/ros/rospack/issues/34>`_) 2.2.0 (2014-01-30) ------------------ * add hash of ROS_PACKAGE_PATH to rospack/rosstack cache filename, remove ROS_ROOT from cache (`#28 <https://github.com/ros/rospack/issues/28>`_) 2.1.22 (2014-01-07) ------------------- * use specific python version catkin has decided on (`#29 <https://github.com/ros/rospack/issues/29>`_) * python 3 compatibility (`#25 <https://github.com/ros/rospack/issues/25>`_, `#27 <https://github.com/ros/rospack/issues/27>`_) * fall back gracefully whe gtest is not available * update package urls 2.1.21 (2013-07-05) ------------------- * honor CATKIN_IGNORE marker file when crawling for packages (`#21 <https://github.com/ros/rospack/issues/21>`_) 2.1.20 (2013-07-03) ------------------- * improve error message to include package names when circular dependency is detected (`#18 <https://github.com/ros/rospack/issues/18>`_) * check for CATKIN_ENABLE_TESTING to enable configure without tests * add '-h' option

po1 mentioned this pull request Feb 15, 2014

Only perform backquote substitution when needed pal-robotics-forks/rospack#1

Merged

dirk-thomas reviewed Feb 15, 2014
View reviewed changes

Paul Mathieu added 2 commits February 15, 2014 20:10

Only perform backquote substitution when needed

7e98549

Spawning shells is an expensive operation. In a lot of cases, there is no need for any substitution. Besides, this does not work on Windows.

Remove trailing spaces in rospack.cpp

e894c99

dirk-thomas added a commit that referenced this pull request Feb 17, 2014

only perform backquote substitution when needed (#34)

27ed245

dirk-thomas closed this Feb 25, 2014

po1 deleted the expand-when-needed branch February 25, 2014 22:40

adolfo-rt mentioned this pull request Jun 29, 2015

Added article about ROS 2.0 in realtime ros2/design#35

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only perform backquote substitution when needed #34

Only perform backquote substitution when needed #34

po1 commented Feb 15, 2014

dirk-thomas Feb 15, 2014

po1 Feb 15, 2014

dirk-thomas Feb 15, 2014

po1 Feb 15, 2014

po1 Feb 15, 2014

dirk-thomas Feb 15, 2014

dirk-thomas Feb 15, 2014

po1 commented Feb 15, 2014

dirk-thomas commented Feb 15, 2014

po1 commented Feb 15, 2014

dirk-thomas commented Feb 15, 2014

tfoote commented Feb 16, 2014

dirk-thomas commented Feb 17, 2014

wjwwood commented Feb 17, 2014

adolfo-rt commented Feb 17, 2014

dirk-thomas commented Feb 17, 2014

adolfo-rt commented Feb 18, 2014

dirk-thomas commented Feb 25, 2014

adolfo-rt commented Feb 26, 2014

Only perform backquote substitution when needed #34

Only perform backquote substitution when needed #34

Conversation

po1 commented Feb 15, 2014

dirk-thomas Feb 15, 2014

Choose a reason for hiding this comment

po1 Feb 15, 2014

Choose a reason for hiding this comment

dirk-thomas Feb 15, 2014

Choose a reason for hiding this comment

po1 Feb 15, 2014

Choose a reason for hiding this comment

po1 Feb 15, 2014

Choose a reason for hiding this comment

dirk-thomas Feb 15, 2014

Choose a reason for hiding this comment

dirk-thomas Feb 15, 2014

Choose a reason for hiding this comment

po1 commented Feb 15, 2014

dirk-thomas commented Feb 15, 2014

po1 commented Feb 15, 2014

dirk-thomas commented Feb 15, 2014

tfoote commented Feb 16, 2014

dirk-thomas commented Feb 17, 2014

wjwwood commented Feb 17, 2014

adolfo-rt commented Feb 17, 2014

dirk-thomas commented Feb 17, 2014

adolfo-rt commented Feb 18, 2014

dirk-thomas commented Feb 25, 2014

adolfo-rt commented Feb 26, 2014