Fix installation of templates for ros2pkg create #87

dhood · 2018-04-03T23:57:46Z

connects to #86
resolves #86

Previously (when not using symlink install), templates were being looked for in ~/ros2_ws/install_isolated/ros2pkg/lib/python3.5/site-packages/ros2pkg/.

I didn't see how to make data_files install to that location (and it might not make sense conceptually) so I'm installing to the share directory instead. Works with symlink or nonsymlink installs now.

dirk-thomas

The patch looks good to me (I haven't tried to install and run it though).

mikaelarguedas

tested with and without symlink install, totally works 👍

mikaelarguedas · 2018-04-04T00:12:36Z

ros2pkg/ros2pkg/api/create.py

-RESOURCE_PATH = os.path.join(os.path.dirname(__file__), '..', 'resource')
+from ros2pkg.api import get_prefix_path
+
+RESOURCE_PATH = os.path.join(get_prefix_path('ros2pkg'), 'share', 'ros2pkg', 'resource')


Is this an absolute path ?
If so, template_path line 71 can just get rid of __file__ as well and just use the resource path directly

yeah I think __file__ maybe didn't belong regardless. will fix

dhood · 2018-04-04T00:35:14Z

CI for linters (thanks for testing manually):

mikaelarguedas · 2018-04-04T00:35:39Z

ros2pkg/setup.py

+        ('share/ros2pkg/resource/package_environment', [
+            'resource/package_environment/package.xml.em',
+        ]),
+    ],
    install_requires=['ros2cli'],
    zip_safe=False,


With __file__ completely removed, this package may very well be zip_safe now (just I hunch, I didn't actually check)

not sure, it has to do with data files too from what I've read, but this page says: "And if the project uses pkg_resources for all its data file access, then C extensions and other data files shouldn’t be a problem at all." so perhaps after 3b38a29 it's fine but I am not sure how to test for sure.

For using pkg_resources though the package should declare a dependency on setuptools since it will be needed at runtime of the package.

zip_safe=True is fine if the code doesn't use __file__ or anything similar to directly read individual files.

Re-review...

dirk-thomas · 2018-04-04T00:56:37Z

ros2pkg/ros2pkg/api/create.py

+from ros2pkg.api import get_prefix_path
+
+RESOURCE_PATH = os.path.abspath(
+    os.path.join(get_prefix_path('ros2pkg'), 'share', 'ros2pkg', 'resource'))


Looking at this again I don't think we should couple the resource lookup with the ament index. Instead use the pkg_resources API for this: like https://github.com/ament/ament_package/blob/28308125089b607f2797abee577371d7ee9a0934/ament_package/templates.py#L24

so install templates with package_data instead of data_files, is that right? I think that's the answer to the "how do I install to site-packges" question I was having, good to know that this exists (and that we don't have to specify each file manually!)

This reverts commit 90556f6.

Will install to e.g. install_isolated/ros2pkg/lib/python3.5/site-packages/ros2pkg/resource which is where it was being looked for previously

mikaelarguedas · 2018-04-05T18:15:15Z

ros2pkg/package.xml

@@ -12,6 +12,8 @@
  <depend>ros2cli</depend>

  <exec_depend>ament_index_python</exec_depend>
+  <exec_depend>python3-empy</exec_depend>
+  <exec_depend>python3-setuptools</exec_depend>


I don't think we declare (in the package.xml) dependency on setuptools for pure python packages (the same way we don't declare a dependency on cmake for ament_cmake packages).

Not sure about listing it in the install_requires section of the setup.py, it seems that some packages do and some don't. Maybe @dirk-thomas can give more details on that point

In a setup.py file I am not sure it makes sense. Because usually that very same file already imports setuptools at the beginning so it would fail already when the dependency is not present.

Once we declare these information in a setup.cfg instead I think it does make sense to list setuptools as a dependency.

I am using the package now at: 3b38a29#diff-97bb991599247d807fe7cf4dad543f62R20
(and generally coping ament_package, for reference)

I based my comment on the content of this repository. the ros2cli package (dependency of this one) uses setuptools and pkg_resources in multiple locations and doesnt declare a dependency on it in either the package.xml or the python files that import the module.
Is the general rule that any package using pkg_resources should declare that dependency in their package.xml ?

Yes, that would make sense.

I guess this should be python3-pkg-resources then

Hmmm I wonder if we are using python3-setuptools at https://github.com/ament/ament_package/blob/28308125089b607f2797abee577371d7ee9a0934/package.xml#L11 because python3-pkg-resources is only valid via apt, not via pip? (from what I can see). Is that it @dirk-thomas ?

Keys used in package.xml are used to query the rosdep database. In general we try to make sure that the rosdep keys resolve to apt packages if such package exist and avoid pip rules as much as we can as we cannot embed these dependencies in our debs.

My guess is that is was an oversight when the dependency was added to ament_package to fix failing downstream packages. As setuptools depends on pkg_resources, the more generic dependency does fix the problem.

I would guess the dependency was even added in to the xml before the rosdep rules existed. Any the assumption was probably at that time there is only setuptools - as you said pkg-resources is not a PyPI package.

ok: minimal, apt-favouring dependency of python3-pkg-resources used in 7c2aef8 👍

dhood · 2018-04-05T18:27:52Z

I put this back in review. github collapsed it but there's an unresolved question in #87 (comment) if anyone has input

dirk-thomas · 2018-04-05T18:32:55Z

ros2pkg/ros2pkg/api/create.py


-RESOURCE_PATH = os.path.join(os.path.dirname(__file__), '..', 'resource')
+RESOURCE_PATH = pkg_resources.resource_filename('ros2pkg', 'resource/')


Why the trailing slash here?

think this usage in general might be off now that I've read this: "Do not use os.path routines to manipulate resource paths, as they are not filesystem paths.". I was going for a minimal change but will replicate https://github.com/ament/ament_package/blob/28308125089b607f2797abee577371d7ee9a0934/ament_package/templates.py#L24 exactly

well aren't I learning a lot today..! resource_filename will return a filesystem path, I think that comment I referenced above is referring to the arguments being passed in, where we aren't using os.path routines anyway. Still, I added 99f0127. If I understood correctly, resource_filename will potentially extract files/directories if the package is installed zipped, and this way it will only extract the files when they're needed, as opposed to the whole resource directory being extracted (or we can swap back).

Prevents resource_filename from extracting whole directory if installation is zipped

dirk-thomas · 2018-04-05T19:18:20Z

ros2pkg/ros2pkg/api/create.py

@@ -65,8 +68,7 @@ def _create_folder(folder_name, base_directory, exist_ok=True):


 def _create_template_file(template_file_name, output_directory, output_file_name, template_config):
-    template_path = os.path.abspath(
-        os.path.join(__file__, RESOURCE_PATH, template_file_name))
+    template_path = _get_template_path(template_file_name)


Maybe move the implementation of _get_template_path here since it is a single function call and only being used in this one location?

yeah, that would make sense f18f246

dhood · 2018-04-06T15:48:47Z

ci just for linters

* have LaunchService.run() return non-0 when there are exceptions * update launch_counters.py example for Windows * fix bug that would cause mismatched asyncio loops in some futures * signal.SIGKILL doesn’t exist on Windows, so emulate it in our Event * remove left over debug statement * fix issue that resulted in spurious asyncio errors in LaunchService test

dhood added 2 commits April 3, 2018 16:47

Move resource dir out of package

90556f6

Install templates to share dir as data_files

05598c6

dhood added the in review Waiting for review (Kanban column) label Apr 3, 2018

dhood self-assigned this Apr 3, 2018

dirk-thomas previously approved these changes Apr 4, 2018

View reviewed changes

mikaelarguedas approved these changes Apr 4, 2018

View reviewed changes

mikaelarguedas reviewed Apr 4, 2018

View reviewed changes

Remove unnecessary __file__ prefix

1f7b17e

mikaelarguedas reviewed Apr 4, 2018

View reviewed changes

dirk-thomas reviewed Apr 4, 2018

View reviewed changes

dhood added 3 commits April 5, 2018 10:53

Revert "Move resource dir out of package"

88a6051

This reverts commit 90556f6.

Swap to package_data installation

3b38a29

Will install to e.g. install_isolated/ros2pkg/lib/python3.5/site-packages/ros2pkg/resource which is where it was being looked for previously

Add exec_depends for third party python packages

484d7bb

mikaelarguedas reviewed Apr 5, 2018

View reviewed changes

dirk-thomas reviewed Apr 5, 2018

View reviewed changes

dhood added 2 commits April 5, 2018 11:42

Set zip_safe to True

ef0ab76

Add/use _get_template_path

99f0127

Prevents resource_filename from extracting whole directory if installation is zipped

dhood force-pushed the fix_ros2pkg_create_install branch from 2aa5b12 to 99f0127 Compare April 5, 2018 19:11

dirk-thomas reviewed Apr 5, 2018

View reviewed changes

dhood added 2 commits April 5, 2018 15:13

Embed _get_template_path contents

f18f246

exec depend on python3-pkg-resources

7c2aef8

dhood requested a review from dirk-thomas April 10, 2018 16:19

dirk-thomas approved these changes Apr 10, 2018

View reviewed changes

dhood merged commit ea3a6b6 into master Apr 10, 2018

dhood deleted the fix_ros2pkg_create_install branch April 10, 2018 17:18

dhood removed the in review Waiting for review (Kanban column) label Apr 10, 2018

dhood mentioned this pull request Apr 10, 2018

ros2 pkg create: resource files not installed #86

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix installation of templates for ros2pkg create #87

Fix installation of templates for ros2pkg create #87

dhood commented Apr 3, 2018 •

edited

Loading

dirk-thomas left a comment

mikaelarguedas left a comment

mikaelarguedas Apr 4, 2018

dhood Apr 4, 2018 •

edited

Loading

dhood Apr 4, 2018

dhood commented Apr 4, 2018

mikaelarguedas Apr 4, 2018

dhood Apr 5, 2018

dirk-thomas Apr 5, 2018

dirk-thomas Apr 5, 2018

dirk-thomas Apr 4, 2018

dhood Apr 4, 2018

mikaelarguedas Apr 5, 2018

dirk-thomas Apr 5, 2018

dhood Apr 5, 2018

mikaelarguedas Apr 5, 2018

dirk-thomas Apr 5, 2018

mikaelarguedas Apr 5, 2018

dhood Apr 5, 2018

mikaelarguedas Apr 5, 2018

dirk-thomas Apr 5, 2018

dhood Apr 5, 2018

dhood commented Apr 5, 2018

dirk-thomas Apr 5, 2018

dhood Apr 5, 2018

dhood Apr 5, 2018

dirk-thomas Apr 5, 2018

dhood Apr 5, 2018

dhood commented Apr 6, 2018


		RESOURCE_PATH = os.path.join(os.path.dirname(__file__), '..', 'resource')
		RESOURCE_PATH = pkg_resources.resource_filename('ros2pkg', 'resource/')

Fix installation of templates for ros2pkg create #87

Fix installation of templates for ros2pkg create #87

Conversation

dhood commented Apr 3, 2018 • edited Loading

dirk-thomas left a comment

Choose a reason for hiding this comment

mikaelarguedas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhood Apr 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhood commented Apr 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhood commented Apr 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhood commented Apr 6, 2018

dhood commented Apr 3, 2018 •

edited

Loading

dhood Apr 4, 2018 •

edited

Loading