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.

Sign up for GitHub

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

Jump to bottom

Make ament_python_install_package() install a flat Python egg #316

Merged
merged 13 commits into from Feb 24, 2021
Merged

Make ament_python_install_package() install a flat Python egg #316

merged 13 commits into from Feb 24, 2021

Conversation

hidmic
Copy link
Contributor

@hidmic hidmic commented Jan 28, 2021

Precisely what the title says. A first stab at addressing #213 and #156.

Also connected to ros2/rosidl#565.

@hidmic
Make ament_python_install_package() install a flat Python .egg
8e38faf 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
This was referenced Jan 28, 2021
Closed
Closed
ivanpauno
ivanpauno reviewed Jan 29, 2021
View reviewed changes
Copy link
Contributor

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

Looking pretty good to me!

I've left some minimal comments

ament_cmake_python/cmake/ament_python_install_package.cmake Outdated
@@ -13,14 +13,18 @@
# limitations under the License.

#
# Install a Python package (and its recursive subpackages).
# Install a Python package (and its recursive subpackages) as a flat Python .egg
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Install a Python package (and its recursive subpackages) as a flat Python .egg
# Install a Python package (and its recursive subpackages) as a flat Python egg.

ament_cmake_python/cmake/ament_python_install_package.cmake Outdated Show resolved Hide resolved
ament_cmake_python/cmake/ament_python_install_package.cmake Show resolved Hide resolved
ament_cmake_python/cmake/ament_python_install_package.cmake Outdated Show resolved Hide resolved
@hidmic
Address peer review comments.
4c518ea 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented Jan 29, 2021

Tested against rclpy, it works flawlessly.

Running full CI, JIC:

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

@hidmic hidmic requested a review from sloretz January 29, 2021 22:34
@hidmic
Include subpackages and data files.
1589dfb 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented Feb 2, 2021 •
edited

Argh, one three little details missing. Running full CI, again:

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

@hidmic hidmic changed the title Make ament_python_install_package() install a flat Python .egg Make ament_python_install_package() install a Python egg Feb 2, 2021
@hidmic hidmic changed the title Make ament_python_install_package() install a Python egg Make ament_python_install_package() install a flat Python egg Feb 2, 2021
@hidmic
Fix package glob expressions
b3d783d 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Always use relative paths
57af8f5 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Normalize setuptools.find_packages() path
a55116e 
It fails on Windows otherwise

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented Feb 9, 2021

@ivanpauno @sloretz PTAL!

@hidmic hidmic requested a review from ivanpauno February 9, 2021 13:15
sloretz
sloretz reviewed Feb 13, 2021
View reviewed changes
ament_cmake_python/cmake/ament_python_install_package.cmake Outdated
where=os.path.normpath('${source_dir}/..'),
include=('${package_name}', '${package_name}.*')),
package_dir={'${package_name}': '${source_dir}'},
package_data={'': ['*.*']}
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is mimicking the install rule copying the entire package dir, then it seems like the pattern should be * instead of *.* to include files without dots.

Might need something like exclude_package_data={'': ['*.pyc', '__pycache__']} to avoid copying any byte code files in the source directory like the install() rule's EXCLUDE patterns, though it seems like that might not be enough if there are byte code files in a subdirectory and exclude_package_data also doesn't support recursive glob patterns pypa/setuptools#1806 . I don't have any ideas on how to fix it, but I'm fine with not fixing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're completely right. My implementation was biased by our use cases.

I think I got it right in 7876939 and a2f1349. PTAL !

ament_cmake_python/cmake/ament_python_install_package.cmake Outdated Show resolved Hide resolved
ament_cmake_python/cmake/ament_python_install_package.cmake Outdated Show resolved Hide resolved
@sloretz
Copy link
Contributor

sloretz commented Feb 13, 2021

Approach LGTM, just some nitpick comments

@hidmic
Add a few missing quotes
7d8cc5c 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Rework how package_data is collected.
7876939 
- Handle files with no extension
- Handle arbitrarily nested subdirectories

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Add tests for find_packages_data()
a2f1349 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented Feb 15, 2021

Running full CI:

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

@hidmic
Fix find_packages_data() tests on Windows.
d6d13a2 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented Feb 17, 2021

CI up to ament_cmake_python, to make sure tests now pass:

  • Windows Build Status

@hidmic hidmic requested a review from sloretz February 17, 2021 17:22
@hidmic
Test byte-compiled Python sources are excluded.
e7be7d4 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Ignore byte-compiled Python files
4db9bb8 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Add test for nested Python package in nested directory
54aeedb 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented Feb 17, 2021

Running CI up to ament_cmake_python to cover the new tests:

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

@hidmic hidmic requested a review from sloretz February 17, 2021 21:45
sloretz
sloretz approved these changes Feb 24, 2021
View reviewed changes
Copy link
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

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

LGTM with green CI (can't currently see CI badges because ci.ros2.org is under maintenance)

@hidmic
Copy link
Contributor Author

hidmic commented Feb 24, 2021

CI's is completely green, merging !

@hidmic hidmic merged commit 42b972e into master Feb 24, 2021
@delete-merged-branch delete-merged-branch bot deleted the hidmic/ament-cmake-python-egg branch February 24, 2021 13:37
@hidmic hidmic mentioned this pull request Feb 25, 2021
Merged
This was referenced Mar 4, 2021
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sloretz sloretz sloretz approved these changes

@ivanpauno ivanpauno Awaiting requested review from ivanpauno

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@hidmic @sloretz @ivanpauno