Skip to content
This repository was archived by the owner on Mar 7, 2018. It is now read-only.

add get_data_files function#2

Merged
dirk-thomas merged 1 commit intomasterfrom
data_files
Jun 28, 2017
Merged

add get_data_files function#2
dirk-thomas merged 1 commit intomasterfrom
data_files

Conversation

@dirk-thomas
Copy link
Copy Markdown
Contributor

@dirk-thomas dirk-thomas commented Jun 28, 2017 •
edited
Loading

The setup function does not allow filenames to be renamed during the install step. Also they should be part of the source tree since the the filename will be added to the manifest.

Hence each package using this function needs to place a file with the name of the package somewhere. By default into the resource subfolder`.

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@dirk-thomas dirk-thomas self-assigned this Jun 28, 2017
This was referenced Jun 28, 2017
Merged
Merged
Merged
nuclearsandwich
nuclearsandwich approved these changes Jun 28, 2017
View reviewed changes
ament_python/data_files.py
data_files = []

# install package manifest
data_files.append(('share/' + package_name, ['package.xml']))
Copy link
Copy Markdown

@nuclearsandwich nuclearsandwich Jun 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this library and function are to be used for explicitly ament_python rather than pure python packages there should indeed always be a package.xml.

Copy link
Copy Markdown
Contributor Author

@dirk-thomas dirk-thomas Jun 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently there is no "pure python" build type. We probably need to revisit the details when that is being introduced.

@nuclearsandwich
Copy link
Copy Markdown

nuclearsandwich commented Jun 28, 2017

ament_tools obviously doesn't biff or the CI wouldn't have passed, but does it behave any differently for python packages with this change or just regenerate the files anyway?

@dirk-thomas
Copy link
Copy Markdown
Contributor Author

dirk-thomas commented Jun 28, 2017

ament_tools obviously doesn't biff or the CI wouldn't have passed, but does it behave any differently for python packages with this change or just regenerate the files anyway?

I am not sure I understand the question. Maybe you can rephrase / clarify it.

@dirk-thomas
Copy link
Copy Markdown
Contributor Author

dirk-thomas commented Jun 28, 2017

I will merge the patch for now as is. Any additional comments can be addressed afterwards.

@dirk-thomas dirk-thomas merged commit e05e9b6 into master Jun 28, 2017
@dirk-thomas dirk-thomas deleted the data_files branch June 28, 2017 05:34
@nuclearsandwich
Copy link
Copy Markdown

nuclearsandwich commented Jun 28, 2017

I am not sure I understand the question. Maybe you can rephrase / clarify it.

The CI run demonstrates that adding this change did not adversely affect source builds. If both ament_tools and setup.py try to place the data files nothing errors out. From looking at the build log (sampled below) it seems like the setup.py created data files are not retained after build. I don't think this is an issue unless there's a disparity between the resources ament_tools creates and the ones in the source, but since they currently both use the same package.xml and the other file is empty and based on the package name I don't think that's anything to worry about.

00:51:06 running install_data
00:51:06 creating /home/rosbuild/ci_scripts/ws/build/demo_nodes_py/build/bdist.linux-x86_64/egg/share
00:51:06 creating /home/rosbuild/ci_scripts/ws/build/demo_nodes_py/build/bdist.linux-x86_64/egg/share/demo_nodes_py
00:51:06 copying package.xml -> /home/rosbuild/ci_scripts/ws/build/demo_nodes_py/build/bdist.linux-x86_64/egg/share/demo_nodes_py
00:51:06 creating /home/rosbuild/ci_scripts/ws/build/demo_nodes_py/build/bdist.linux-x86_64/egg/share/ament_index
00:51:06 creating /home/rosbuild/ci_scripts/ws/build/demo_nodes_py/build/bdist.linux-x86_64/egg/share/ament_index/resource_index
00:51:06 creating /home/rosbuild/ci_scripts/ws/build/demo_nodes_py/build/bdist.linux-x86_64/egg/share/ament_index/resource_index/packages

@dirk-thomas
Copy link
Copy Markdown
Contributor Author

dirk-thomas commented Jun 28, 2017

I don't think this is an issue unless there's a disparity between the resources ament_tools creates and the ones in the source, but since they currently both use the same package.xml and the other file is empty and based on the package name I don't think that's anything to worry about.

If the two versions of the same file would be different ament_tools would not overwrite the file: https://github.com/ament/ament_tools/blob/cd0f3b3569a79974531a918ac683a07ff4cbb504/ament_tools/helper.py#L255-L259

@nuclearsandwich
Copy link
Copy Markdown

nuclearsandwich commented Jun 28, 2017

If the two versions of the same file would be different ament_tools would not overwrite the file: https://github.com/ament/ament_tools/blob/cd0f3b3569a79974531a918ac683a07ff4cbb504/ament_tools/helper.py#L255-L259

Cool. Hadn't seen that.

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

Reviewers

2 more reviewers

@wjwwood wjwwood wjwwood approved these changes

@nuclearsandwich nuclearsandwich nuclearsandwich approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@dirk-thomas dirk-thomas

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dirk-thomas @nuclearsandwich @wjwwood