[ament_cmake_python] ament_cmake_python_get_python_install_dir public by MaximilienNaveau · Pull Request #300 · ament/ament_cmake

MaximilienNaveau · 2020-10-28T18:38:04Z

Description

This PR has for purpose to answer the issue #277

What has been changed?

A new macro ament_get_python_install_dir allow the user to access the python installation directory.

example of usage

ament_get_python_install_dir(path_to_python_install_dir)
install(python_bindings_target DESTINATION ${path_to_python_install_dir}/${PROJECT_NAME})

clalancette

Seems reasonable to me. I'll run CI on it next.

clalancette · 2020-10-29T13:46:42Z

CI:

Linux
Linux-aarch64
macOS
Windows

hidmic · 2020-10-30T20:15:31Z

ament_cmake_python/ament_cmake_python-extras.cmake

 endmacro()

-macro(_ament_cmake_python_get_python_install_dir)
+macro(ament_cmake_python_get_python_install_dir)


@MaximilienNaveau now that this is public API, a documentation block would be nice.

I recommend making this a function() too with an output variable to avoid other variables it sets (_python_code, _output, _result) from being visible to the code that calls this. I also recommend shortening the name.

This

_ament_cmake_python_get_python_install_dir()

would become this:

ament_get_python_install_dir(PYTHON_INSTALL_DIR)

I do agree I will do the change whenever I have some time!

@MaximilienNaveau friendly ping

Sorry I have been under water lately I will try to do this next week!

MaximilienNaveau · 2020-11-25T10:57:05Z

Dear @ALL I hopefully took all comments into account in the last commit,
Please let me know if something is wrong.

…ir a public interface. Signed-off-by: Maximilien Naveau <maximilien.naveau@tuebingen.mpg.de>

- Create a file ament_cmake_python/cmake/ament_get_python_install_dir.cmake containing a macro to access the python installation directory. - In ament_cmake_python/ament_cmake_python-extras.cmake restore the way to find the python directory, and include the ament_cmake_python/cmake/ament_get_python_install_dir.cmake Signed-off-by: Maximilien Naveau <maximilien.naveau@tuebingen.mpg.de>

Signed-off-by: Maximilien Naveau <maximilien.naveau@tuebingen.mpg.de>

hidmic · 2020-12-01T19:59:38Z

ament_cmake_python/cmake/ament_get_python_install_dir.cmake

@@ -0,0 +1,29 @@
+# Copyright 2014 Open Source Robotics Foundation, Inc.


@MaximilienNaveau nit:

Suggested change

# Copyright 2014 Open Source Robotics Foundation, Inc.

# Copyright 2014-2020 Open Source Robotics Foundation, Inc.

hidmic · 2020-12-01T20:00:29Z

ament_cmake_python/cmake/ament_get_python_install_dir.cmake

+#   Python installation directory.
+# :type package_name: string
+#
+macro(ament_get_python_install_dir python_install_dir_out)


@MaximilienNaveau why not a function, as @sloretz suggested?

Because for now all the public "function" defined in this package are implemented as macros:

https://github.com/ament/ament_cmake/blob/master/ament_cmake_python/cmake/ament_python_install_module.cmake

https://github.com/ament/ament_cmake/blob/master/ament_cmake_python/cmake/ament_python_install_package.cmake
The function is used with an underscore (function(_function_name args)) which indicates them as private in this package.

I can still use a function if you really want to.

Note that none of those macros pollutes the calling scope. I'd recommend using a function for this case.

I did not see that, I did the modification.

Signed-off-by: Maximilien Naveau <maximilien.naveau@tuebingen.mpg.de>

hidmic

LGTM once CI goes green!

Linux
Linux-aarch64
macOS
Windows

MaximilienNaveau · 2020-12-08T17:52:56Z

LGTM once CI goes green!

* Linux [![Build Status](https://camo.githubusercontent.com/e2876de455e260538d341f2332b2c58b33096e883171dc81ec5b153871e8cde1/687474703a2f2f63692e726f73322e6f72672f6275696c645374617475732f69636f6e3f6a6f623d63695f6c696e7578266275696c643d3133323039)](http://ci.ros2.org/job/ci_linux/13209/)

* Linux-aarch64 [![Build Status](https://camo.githubusercontent.com/bcc5670e4fe22b2d6bd43f620f6b73d68db2416c7327d353fd911e11f164f988/687474703a2f2f63692e726f73322e6f72672f6275696c645374617475732f69636f6e3f6a6f623d63695f6c696e75782d61617263683634266275696c643d38313430)](http://ci.ros2.org/job/ci_linux-aarch64/8140/)

* macOS [![Build Status](https://camo.githubusercontent.com/8c12935526b4e1b50b6a87fe788ed94162c6567acbf3f13e9ce16e18aeedee47/687474703a2f2f63692e726f73322e6f72672f6275696c645374617475732f69636f6e3f6a6f623d63695f6f7378266275696c643d3130393235)](http://ci.ros2.org/job/ci_osx/10925/)

* Windows [![Build Status](https://camo.githubusercontent.com/3a644035d7a4f36eb2db6ab4b16019a601f33affad2dc927d39e4f6b4ea97fae/687474703a2f2f63692e726f73322e6f72672f6275696c645374617475732f69636f6e3f6a6f623d63695f77696e646f7773266275696c643d3133323236)](http://ci.ros2.org/job/ci_windows/13226/)

So should it be merged?

clalancette · 2020-12-08T18:41:58Z

So should it be merged?

Yes. The yellow builds are known flakey tests, so I'll go ahead and merge this. Thanks for the contribution.

MaximilienNaveau force-pushed the master branch from ac23800 to 52727d8 Compare October 28, 2020 18:39

clalancette approved these changes Oct 29, 2020

View reviewed changes

hidmic reviewed Oct 30, 2020

View reviewed changes

clalancette added more-information-needed Further information is required requires-changes labels Nov 18, 2020

Maximilien Naveau added 3 commits November 25, 2020 12:00

[ament_cmake_python] make the ament_cmake_python_get_python_install_d…

b3e3139

…ir a public interface. Signed-off-by: Maximilien Naveau <maximilien.naveau@tuebingen.mpg.de>

[ament_cmake_python] fix previous commit

9bbf02d

Signed-off-by: Maximilien Naveau <maximilien.naveau@tuebingen.mpg.de>

MaximilienNaveau force-pushed the master branch from ee3f4a4 to 9bbf02d Compare November 25, 2020 11:00

MaximilienNaveau requested review from hidmic and sloretz November 25, 2020 11:01

hidmic reviewed Dec 1, 2020

View reviewed changes

update copyright

d049d1e

Signed-off-by: Maximilien Naveau <maximilien.naveau@tuebingen.mpg.de>

MaximilienNaveau force-pushed the master branch from 8071fd6 to d049d1e Compare December 2, 2020 14:58

update macro in function

d800786

Signed-off-by: Maximilien Naveau <maximilien.naveau@tuebingen.mpg.de>

MaximilienNaveau force-pushed the master branch from 09f64e4 to d800786 Compare December 2, 2020 17:34

hidmic approved these changes Dec 2, 2020

View reviewed changes

clalancette merged commit 7f76158 into ament:master Dec 8, 2020

hidmic mentioned this pull request Mar 11, 2021

[ament_cmake_python] expose the _ament_cmake_python_get_python_install_dir as public? #277

Closed

		@@ -0,0 +1,29 @@
		# Copyright 2014 Open Source Robotics Foundation, Inc.

	# Copyright 2014 Open Source Robotics Foundation, Inc.
	# Copyright 2014-2020 Open Source Robotics Foundation, Inc.

Conversation

MaximilienNaveau commented Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

What has been changed?

example of usage

Uh oh!

clalancette left a comment

Choose a reason for hiding this comment

Uh oh!

clalancette commented Oct 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaximilienNaveau commented Nov 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hidmic left a comment

Choose a reason for hiding this comment

Uh oh!

MaximilienNaveau commented Dec 8, 2020

Uh oh!

clalancette commented Dec 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MaximilienNaveau commented Oct 28, 2020 •

edited

Loading