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

Correct CMake Logic and update cpplint to Python3 #117

Merged
merged 7 commits into from
Nov 10, 2020
Merged

Correct CMake Logic and update cpplint to Python3 #117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c90bbd9
Update cpplint to python3 compatible-version
mjcarroll Oct 9, 2020
ef66797
Find Python3 if available
mjcarroll Oct 9, 2020
ed595cb
Split targets but maintain codecheck
mjcarroll Oct 9, 2020
084ab9a
Update cmake/IgnPython.cmake
mjcarroll Oct 12, 2020
ebc2fca
Add Ignition-specific default filters and suppressions
mjcarroll Oct 26, 2020
3785150
Don't do C+11 checks
mjcarroll Oct 26, 2020
32c7f59
Add permalink
mjcarroll Oct 26, 2020
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
18 changes: 8 additions & 10 deletions cmake/IgnCodeCheck.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,15 @@
# Setup the codecheck target, which will run cppcheck and cppplint.
function(ign_setup_target_for_codecheck)
include(IgnPython)

find_program(CPPCHECK_PATH cppcheck)
find_program(PYTHON_PATH python)
find_program(FIND_PATH find)

if(NOT CPPCHECK_PATH)
message(STATUS "The program [cppcheck] was not found! Skipping codecheck setup")
return()
endif()

if(NOT PYTHON_PATH)
message(STATUS "python not found! Skipping codecheck setup.")
return()
endif()

if(NOT FIND_PATH)
message(STATUS "The program [find] was not found! Skipping codecheck setup.")
return()
Expand All @@ -37,17 +32,20 @@ function(ign_setup_target_for_codecheck)

message(STATUS "Adding codecheck target")

# Setup the codecheck target
add_custom_target(codecheck

add_custom_target(cppcheck
# First cppcheck
COMMAND ${CPPCHECK_PATH} ${CPPCHECK_BASE} ${CPPCHECK_EXTRA} -I ${CPPCHECK_INCLUDE_DIRS} ${CPPCHECK_RULES} `${CPPCHECK_FIND}`

# Second cppcheck
COMMAND ${CPPCHECK_PATH} ${CPPCHECK_BASE} --enable=missingInclude `${CPPCHECK_FIND}`
)

add_custom_target(cpplint
# cpplint cppcheck
COMMAND python ${IGNITION_CMAKE_CODECHECK_DIR}/cpplint.py --extensions=cc,hh --quiet `${CPPCHECK_FIND}`
COMMAND ${PYTHON_EXECUTABLE} ${IGNITION_CMAKE_CODECHECK_DIR}/cpplint.py --extensions=cc,hh --quiet `${CPPCHECK_FIND}`
)

add_custom_target(codecheck
DEPENDS cpplint cppcheck
)
endfunction()
25 changes: 25 additions & 0 deletions cmake/IgnPython.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# Copyright 2020 Open Source Robotics Foundation, Inc.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

# Copied from ament/ament_cmake: ament_cmake/ament_cmake_core/cmake/core/python.cmake

set(PYTHON_VERSION "" CACHE STRING
"Specify specific Python version to use ('major.minor' or 'major')")

# if not specified otherwise use Python 3
if(NOT PYTHON_VERSION)
set(PYTHON_VERSION "3")
endif()

find_package(PythonInterp ${PYTHON_VERSION} REQUIRED)
Copy link
Member

Choose a reason for hiding this comment

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

does this mean python is now a required dependency of ignition-cmake2? the homebrew bottle build of 2.6.0 in osrf/homebrew-simulation#1222 is failing

Build Status https://build.osrfoundation.org/job/generic-release-homebrew_triggered_bottle_builder/label=osx_mojave/124/

Copy link
Member

Choose a reason for hiding this comment

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

I think we might want this to be QUIET instead of REQUIRED

Copy link
Contributor

Choose a reason for hiding this comment

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

does this mean python is now a required dependency of ignition-cmake2?

I think that this is what this line implies, although I'm not sure if we meant to create this dependency or not.

I think we might want this to be QUIET instead of REQUIRED

If we do need python though, what would the implications be of replacing REQUIRED with QUIET? I'm wondering if marking this QUIET can cause issues elsewhere if python is not found. @mjcarroll, what are your thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's an optional dependency that's only needed for optional modules (e.g. testing?), then whatever modules depend on it should check ${PythonInterp_FOUND} to conditionally build only if this package was found. If the package was found, then the module should skip being built.

Copy link
Member

Choose a reason for hiding this comment

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

we can continue the discussion in #132, which proposes to revert python3 to an optional dependency

3 changes: 2 additions & 1 deletion cmake/IgnUtils.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1676,7 +1676,8 @@ macro(ign_build_tests)

# Find the Python interpreter for running the
# check_test_ran.py script
find_package(PythonInterp QUIET)

include(IgnPython)

# Build all the tests
foreach(target_name ${test_list})
Expand Down
Loading