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

Resolve some todos #103

Merged
merged 4 commits into from
Aug 8, 2017
+31 −17
Merged

Resolve some todos #103

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
947e92f
remove obsolete todos
dirk-thomas Aug 7, 2017
7e36dc3
move todo to line with comment
dirk-thomas Aug 7, 2017
9810cac
use file(GLOB LIST_DIRECTORIES
dirk-thomas Aug 7, 2017
372f81e
compile installed Python modules and packages by default, add option …
dirk-thomas Aug 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions ament_cmake_core/cmake/index/ament_cmake_index_package_hook.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,3 @@

# register package name for index
ament_index_register_package()

# TODO register plugins
1 change: 0 additions & 1 deletion ament_cmake_core/cmake/index/ament_index_get_prefix_path.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ function(ament_index_get_prefix_path var)

# Remove CMAKE_INSTALL_PREFIX if it is in the list of paths to search,
# and add it to the list at the front
# TODO(dirk-thomas) check if this can be removed or the insert can be done
list(REMOVE_ITEM prefix_path "${CMAKE_INSTALL_PREFIX}")
list(INSERT prefix_path 0 "${CMAKE_INSTALL_PREFIX}")

Expand Down
7 changes: 3 additions & 4 deletions ament_cmake_core/cmake/index/ament_index_get_resources.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,15 +54,14 @@ function(ament_index_get_resources var resource_type)
set(all_resources "")
foreach(path IN LISTS paths_to_search)
set(resource_index_path "${path}/share/ament_index/resource_index")
# TODO use LIST_DIRECTORIES false in GLOB call when available in CMake 3.5
file(GLOB resources
LIST_DIRECTORIES FALSE
RELATIVE "${resource_index_path}/${resource_type}"
"${resource_index_path}/${resource_type}/*")
foreach(resource IN LISTS resources)
string(SUBSTRING "${resource}" 0 1 resource_char0)
# Ignore all subdirectories, and any files starting with a dot
if((NOT IS_DIRECTORY "${resource_index_path}/${resource_type}/${resource}")
AND (NOT resource_char0 STREQUAL "."))
# Ignore any files starting with a dot
if(NOT resource_char0 STREQUAL ".")
list_append_unique(all_resources ${resource})
endif()
endforeach()
Expand Down
3 changes: 1 addition & 2 deletions ament_cmake_export_libraries/cmake/ament_cmake_export_libraries-extras.cmake.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,8 +134,7 @@ if(NOT _exported_library_names STREQUAL "")
endwhile()
endif()

# deduplicate @PROJECT_NAME@_LIBRARIES
# TODO(dirk-thomas) deduplicate @PROJECT_NAME@_LIBRARIES
# while maintaining library order
# as well as build configuration keywords
# as well as linker flags
# TODO
3 changes: 1 addition & 2 deletions ament_cmake_export_libraries/cmake/ament_cmake_export_libraries_package_hook.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,10 @@
# See the License for the specific language governing permissions and
# limitations under the License.

# deduplicate _AMENT_EXPORT_LIBRARIES and _AMENT_EXPORT_LIBRARY_NAMES
# TODO(dirk-thomas) deduplicate _AMENT_EXPORT_LIBRARIES and _AMENT_EXPORT_LIBRARY_NAMES
# while maintaining library order
# as well as build configuration keywords
# as well as linker flags
# TODO

# generate and register extra file for libraries
set(_generated_extra_file
Expand Down
2 changes: 0 additions & 2 deletions ament_cmake_nose/cmake/ament_add_nose_test.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,5 +133,3 @@ function(_ament_add_nose_test testname path)
LABELS "nose"
)
endfunction()

# TODO provide function to register all found tests separately
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what this TODO means exactly, and where has the function it refers to been implemented. Can you provide a pointer to that function ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea was to introspect nosetest and register each individual test function as a separate CTest. I don't think we will purpose the idea though since that make reporting the results in separate JUnit files much more difficult and the runtime would be worse since each test would use a separate Python interpreter.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could such a function provide the ability to flag / label test functions independently (in the vein of what is suggested in ros2/build_farmer#41 (comment)) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests can already be labels within Python. No need for CTest to get involved for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 cool
@nuclearsandwich FYI

15 changes: 13 additions & 2 deletions ament_cmake_python/cmake/ament_python_install_module.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,16 @@
# :param DESTINATION_SUFFIX: the base package to install the module to
# (default: empty, install as a top level module)
# :type DESTINATION_SUFFIX: string
# :param SKIP_COMPILE: if set do not compile the installed module
# :type SKIP_COMPILE: option
#
macro(ament_python_install_module)
_ament_cmake_python_register_environment_hook()
_ament_cmake_python_install_module(${ARGN})
endmacro()

function(_ament_cmake_python_install_module module_file)
cmake_parse_arguments(ARG "" "DESTINATION_SUFFIX" "" ${ARGN})
cmake_parse_arguments(ARG "SKIP_COMPILE" "DESTINATION_SUFFIX" "" ${ARGN})
if(ARG_UNPARSED_ARGUMENTS)
message(FATAL_ERROR "ament_python_install_module() called with unused "
"arguments: ${ARG_UNPARSED_ARGUMENTS}")
Expand Down Expand Up @@ -55,7 +57,16 @@ function(_ament_cmake_python_install_module module_file)
FILES "${module_file}"
DESTINATION "${destination}"
)
# TODO optionally compile Python file
if(NOT ARG_SKIP_COMPILE)
# compile Python files
install(CODE
"execute_process(
COMMAND
\"${PYTHON_EXECUTABLE}\" \"-m\" \"compileall\"
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: should this accept a custom executable like add_nose_test ? (same below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could, but until there is a use case when this is needed I wouldn't implement it.

Copy link
Contributor

Choose a reason for hiding this comment

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

fair enough, windows will tell us soon enough if we need to pass a debug interpreter here

\"${CMAKE_INSTALL_PREFIX}/${destination}/${module_file}\"
)"
)
endif()

if(destination IN_LIST AMENT_CMAKE_PYTHON_INSTALL_INSTALLED_NAMES)
message(FATAL_ERROR "ament_python_install_module() a Python module file "
Expand Down
15 changes: 13 additions & 2 deletions ament_cmake_python/cmake/ament_python_install_package.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,16 @@
# :param PACKAGE_DIR: the path to the Python package directory (default:
# <package_name> folder relative to the CMAKE_CURRENT_LIST_DIR)
# :type PACKAGE_DIR: string
# :param SKIP_COMPILE: if set do not compile the installed package
# :type SKIP_COMPILE: option
#
macro(ament_python_install_package)
_ament_cmake_python_register_environment_hook()
_ament_cmake_python_install_package(${ARGN})
endmacro()

function(_ament_cmake_python_install_package package_name)
cmake_parse_arguments(ARG "" "PACKAGE_DIR" "" ${ARGN})
cmake_parse_arguments(ARG "SKIP_COMPILE" "PACKAGE_DIR" "" ${ARGN})
if(ARG_UNPARSED_ARGUMENTS)
message(FATAL_ERROR "ament_python_install_package() called with unused "
"arguments: ${ARG_UNPARSED_ARGUMENTS}")
Expand Down Expand Up @@ -57,7 +59,16 @@ function(_ament_cmake_python_install_package package_name)
PATTERN "*.pyc" EXCLUDE
PATTERN "__pycache__" EXCLUDE
)
# TODO(dirk-thomas): optionally compile Python files
if(NOT ARG_SKIP_COMPILE)
# compile Python files
install(CODE
"execute_process(
COMMAND
\"${PYTHON_EXECUTABLE}\" \"-m\" \"compileall\"
\"${CMAKE_INSTALL_PREFIX}/${PYTHON_INSTALL_DIR}/${package_name}\"
)"
)
endif()

if(package_name IN_LIST AMENT_CMAKE_PYTHON_INSTALL_INSTALLED_NAMES)
message(FATAL_ERROR
Expand Down