Add ament package for pclint #98

jpsamper2009 · 2018-04-02T17:46:20Z

As promised, here is a copy of our ament_pclint and ament_cmake_pclint packages. I haven't made any changes yet (not even the licenses) because I figure it'll have to go through at least one round of review anyway.
Neither of these will work if you don't have pclint plus installed (you can buy a license at http://www.gimpel.com)
The configuration files are modified versions of the ones found here: http://www.gimpel.com/html/ptch90.htm

jpsamper2009 · 2018-04-02T17:48:40Z

ament_pclint/ament_pclint/main.py

+
+
+def find_executable(file_name, additional_paths=None):
+    if sys.platform == 'linux':


There's a small hack here because the binaries pclp64 is the Windows version, while linux and osx are distributed pclp64_linux and pclp64_osx.

dirk-thomas · 2018-04-02T17:53:23Z

ament_cmake_pclint/package.xml

+    code using PCLint.
+  </description>
+  <maintainer email="jp.samper@apex.ai">Juan Pablo Samper</maintainer>
+  <license>Apex.AI Proprietary License</license>


From this patch it is unclear what this license is. I think each package needs at least a LICENSE file.

@dirk-thomas It will probably become Apache 2.0 (or another well-known license), I just copy-pasted the code for now, so you could start looking at the content.

dirk-thomas

Current the linter invocation fails when pclint isn't available. That would prohibit it from being added as a "default" linter. I think it would be good to have it as a default linter in order to check for it when pclint is available. Therefore I would suggest to gracefully skip linting when pclint isn't available instead.

dirk-thomas · 2018-04-02T20:11:31Z

ament_cmake_pclint/cmake/ament_pclint.cmake

+
+  if(INCLUDED_DIRS)
+      list(APPEND cmd "--include-directories" ${INCLUDED_DIRS})
+      set(_INCLUDE_DIRS_FLAG_SET "Found")


Nitpick: indentation.

Same below.

dirk-thomas · 2018-04-02T20:12:42Z

ament_cmake_pclint/cmake/ament_pclint.cmake

+
+  set(cmd "${ament_pclint_BIN}" ${ARG_UNPARSED_ARGUMENTS})
+
+  get_property(INCLUDED_DIRS DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR} PROPERTY INCLUDE_DIRECTORIES)


How does it handle the case that no global include directories are being used but target specific include directories instead?

Same for compiler definitions below.

I'm not 100% I understand the question, but I think what I meant to do here was gather all the directories included through cmake's include_directories. The idea is that you need to tell pclint where to look for header files, not unlike how you have to tell cmake where to find header files.

It's likely that I'm misusing or misunderstanding something in cmake, though.

Similarly, we want to give pclint compiler definitions added through target_compile_definitions, but again, I may be misunderstanding something fundamental about cmake.

The CMake code might set these information on a per-targetg level using e.g. target_include_directories and/or target_compile_definitions. In that case the global variables might not be sufficient.

I see what you're saying now. I was assuming that any headers are included with include_directories

What I'm thinking is we need to add PCLINT_INCLUDE_DIRS and PCLINT_COMPILE_DEFS options to ament_pclint or do you know of something better to get information added via target_*()?

Basically, pclint needs access to the exact same directories as the add_executable or add_library functions.

I don't see a better way then to let the user pass in the necessary flags manually. That can be either the include dirs directory or a list of targets from which the include dirs are being extracted.

Beside supporting these information as arguments in the CMake function the user also needs to be able to set some CMake variables for the case where the linter is being invoked implicitly by the ament_lint_auto package.

I was considering how to do this as well. @jpsamper2009 is there not a way to retrieve all of the includes and flags for a particular target rather than the global scope in CMake?

I think what you want to do is:

get_target_property(TARGET_INC ${PROJECT_NAME} INCLUDE_DIRECTORIES) message(STATUS "INCLUDES: " ${TARGET_INC}) get_target_property(TARGET_FLAGS ${PROJECT_NAME} COMPILE_FLAGS) message(STATUS "FLAGS: " ${TARGET_FLAGS}) get_target_property(TARGET_DEF ${PROJECT_NAME} COMPILE_DEFINITIONS) message(STATUS "DEFINITIONS: " ${TARGET_DEF})

And then use those generated variables to pass into pclint.

Upon further reflection, I think that the appropriate way to handle ament_cmake_pclint is to add a CMake function that takes a target as an argument. From there, we can retrieve the list of sources, includes, and compiler definitions/flags that are used on that target, to make the linting maximally effective.

This would also allow us to exclude things like unit tests, etc from pclint checking.

@mjcarroll I hadn't seen your comment above, so for now I've just added function arguments to allow the user to specify the include directories explicitly.

Do you guys have an example of a package with target-specific include directories and compiler flags?

dirk-thomas · 2018-04-02T20:13:25Z

ament_cmake_pclint/cmake/ament_pclint.cmake

+  endif()
+
+  set(cmd ${cmd} PARENT_SCOPE)
+


Nitpick: remove the empty lines right after function() and before endfunction().

dirk-thomas · 2018-04-02T20:17:55Z

ament_cmake_pclint/package.xml

+<?xml-model href="http://download.ros.org/schema/package_format2.xsd" schematypens="http://www.w3.org/2001/XMLSchema"?>
+<package format="2">
+  <name>ament_cmake_pclint</name>
+  <version>0.0.3</version>


The version should match the other packages in this repo which are currently 0.4.0.

Same in the other package as well as the setup.py file.

dirk-thomas · 2018-04-02T20:20:45Z

ament_pclint/package.xml

+  <maintainer email="jp.samper@apex.ai">Juan Pablo Samper</maintainer>
+  <license>Apex.AI Proprietary License</license>
+
+  <exec_depend>pclp64</exec_depend>


For the release this needs to be a valid rosdep key. If that is not feasible the dependency needs to be removed.

My guess is this won't be the case since it's a proprietary license, so I'll remove.

dirk-thomas · 2018-04-02T20:31:01Z

ament_pclint/ament_pclint/main.py

+             ', '.join(["'.%s'" % e for e in extensions]))
+    parser.add_argument(
+        '--language',
+        help="Force pclint to analyze files as specified language. Supported languages c or cpp")


Could this provide a choices?

dirk-thomas · 2018-04-02T20:31:50Z

ament_pclint/ament_pclint/main.py

+    if os.name == 'nt':
+        parser.add_argument(
+            '--pclint-config-dir',
+            nargs=1,


The default value of nargs is 1 so no need to specify it explicitly.

Same below.

dirk-thomas · 2018-04-02T20:33:05Z

ament_pclint/ament_pclint/main.py

+
+    # Add include directories from arguments
+    if args.include_directories is not None:
+        for directory in args.include_directories:


Suggestion to avoid the extra conditional:

for directory in (args.include_directories or []):

Same below.

dirk-thomas · 2018-04-02T20:34:05Z

ament_pclint/ament_pclint/main.py

+        # the number of cores cannot be determined, don't parallelize
+        pass
+
+    result = ['' for f in files]


Suggestion: result = [''] * len(files)

Same below.

dirk-thomas · 2018-04-02T20:36:52Z

ament_pclint/ament_pclint/main.py

+            '--pclint-config-dir',
+            nargs=1,
+            type=str,
+            default=[os.path.dirname(os.path.realpath(__file__)) + "/config/msvc"],


Please use single quotes rather than double quotes where possible without escaping.

Same below.

I should probably be using os.path.join, no?

Even better 👍

jpsamper2009 · 2018-04-04T01:44:44Z

@dirk-thomas Do you have a suggestion on how to gracefully skip this test if pclint is not available? I'm thinking I can either check at the cmake level if the executable exists, and if it doesn't print a Warning and continue. I could also just do it at the ament_pclint.py level where if no executable is found, I print a warning and exit with return code 0.

dirk-thomas · 2018-04-04T17:07:46Z

I would propose to set the DISABLED property on the CMake test when pclint was not found without printing a warning. Otherwise when the linter is added to the defaults every user would see a warning for every package when pclint is not available. While this new property is only available as of CMake 3.9 that should be fine since the next release can bump the minimum required version of CMake since Bionic ships with 3.10.

For the command line tool I would rather return a non-zero error code when pclint is unavailable. Passing without actually running the linter doesn't sounds like a good idea to me.

jpsamper2009 · 2018-04-16T21:19:43Z

@mjcarroll Are you using this package? Do you have any feedback?

mjcarroll · 2018-04-16T21:52:19Z

@jpsamper2009 I have been using it (your internal version, I need to check against this one). My biggest things were the exit without the executable being found, and the handling of the compiler flags.

I was also looking into retrieving the target flags for the open-source cppcheck tool for the open-source CI side.

jpsamper2009 · 2018-05-01T18:14:11Z

@mjcarroll @dirk-thomas I've reworked this a bit to get rid of some unused parts and address your suggestions. I'm afraid I didn't see #98 (comment) until just now, but maybe the current solution is sufficient to merge this, and we can work on reading information from a target on a separate issue? It would be good to get some people using it also to see what use cases we actually have instead of implementing a bunch of features that then go unused.

dirk-thomas · 2018-05-01T18:18:34Z

We have to hold this anyway until we have moved to Ubuntu 18.04 since currently we only have CMake 3.5 available on 16.04 and this package require 3.9.

mjcarroll · 2018-05-01T18:48:22Z

What 3.9 features is this using?

jpsamper2009 · 2018-05-01T19:03:56Z

@mjcarroll The second commit is using the DISABLE property which is only available in 3.9 (#98 (comment)). As long as you have pclint installed, you should be able to use the first commit.

@dirk-thomas Ack

mjcarroll · 2018-05-17T17:11:06Z

This has been moved back to in progress. Because we are going to try to keep some Xenial compatibility, we need to find a way to accomplish the DISABLE property without using cmake 3.9

dirk-thomas · 2018-05-17T17:12:02Z

(or just skip creating the test)

jpsamper2009 · 2018-05-19T22:19:06Z

@mjcarroll @dirk-thomas I have added a check to skip the test creation and print a warning if the executable is not found: (c3eaaf4)

I have also put the CMake 3.9 solution into a different branch in case we want to go back to that version eventually: jpsamper2009@e293644

dirk-thomas · 2018-05-23T15:46:58Z

ament_pclint/setup.py

+    author_email='jp.samper@apex.ai',
+    maintainer='Juan Pablo Samper',
+    maintainer_email='jp.samper@apex.ai',
+    url='https://gitlab.com/ApexAI/ament_lint',


Should this point to this repo instead?

Good catch. Fixed.

dirk-thomas · 2018-05-23T15:47:01Z

ament_pclint/setup.py

+    long_description="""\
+The ability to perform static code analysis on C/C++ code using PCLint
+and generate xUnit test result files.""",
+    license='Apex AI',


This information doesn't match the package manifest which declares Apache 2.

Yeah, too many places to change it :). Fixed now.

- Use config file with permissive license: http://www.gimpel.com/html/pub90/au-misra3.lnt

jpsamper2009 · 2018-06-05T18:56:40Z

I've just pushed a change to one of the pclint config files. The file is available online ( http://www.gimpel.com/html/pub90/au-misra3.lnt), but the version we were using had a less permissive license. I've instead adjusted the configuration linked above.

dirk-thomas · 2018-06-14T17:26:02Z

@mjcarroll Please squash merge in the future.

mjcarroll · 2018-06-15T00:17:39Z

@dirk-thomas Sorry about that, I must have misclicked.

serge-nikulin · 2019-05-21T18:26:48Z

@jpsamper2009,

FYI (unless you guys already fixed it)

I finally found the option to check local headers we missed back then: +libclass(angle). See Chapter 5 in the PCLint manual for details. Add this option to both c++.lnt and c99.lnt files.

Also, I use a forced check now for all headers even if they are not used by local sources (via creating throw-away trivial users). See below the relevant fragment from my main.py file.

# For each header file create a temp .cpp file that includes it via ""
# This way PCLint wil check all header files even for pure interface libraries
def prepare_source_files(source_files, include_files):
    tempdir = tempfile.TemporaryDirectory(prefix='ament_pclintsa_')
    for header_name in include_files:
        if header_name.startswith('include/'):
            # A hack which must be fixed
            # remove "include/" prefix
            header_name = header_name[8:]
        header_name = header_name.replace('\\', '/')
        src_name = os.path.join(tempdir.name, os.path.basename(header_name) + '.cpp')
        with open(src_name, 'w') as f:
            f.write('#include "' + header_name + '"\n')
        source_files.append(src_name)
    # return tempdir too so it will be deleted on the script's end
    return source_files, tempdir

dirk-thomas · 2019-05-21T19:20:17Z

@serge-nikulin This ticket has been closed for almost a year. The chances your comment will get lost is very high. Please either create a pull request with your proposed fragment or open an issue with the information (with a strong preference for the former).

jpsamper2009 commented Apr 2, 2018

View reviewed changes

dirk-thomas reviewed Apr 2, 2018

View reviewed changes

jpsamper2009 force-pushed the ament_pclint branch from e5fefde to ebebe60 Compare April 4, 2018 01:15

mikaelarguedas added in progress Actively being worked on (Kanban column) requires-changes labels Apr 5, 2018

mjcarroll mentioned this pull request Apr 6, 2018

MISRA meta-ticket ros2/ros2#475

Closed

10 tasks

jpsamper2009 force-pushed the ament_pclint branch from ebebe60 to a05ad21 Compare April 8, 2018 00:57

Add ament_pclint and ament_cmake_pclint packages

5e5e6b1

jpsamper2009 force-pushed the ament_pclint branch from 2b7483e to e293644 Compare May 1, 2018 18:07

dirk-thomas mentioned this pull request May 1, 2018

Target bionic (meta) ros2/ros2#481

Closed

31 tasks

dirk-thomas added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) requires-changes labels May 1, 2018

mikaelarguedas added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels May 17, 2018

jpsamper2009 force-pushed the ament_pclint branch from e293644 to 5e5e6b1 Compare May 19, 2018 21:02

Skip pclint creation if pclint executable not found

c3eaaf4

dirk-thomas reviewed May 23, 2018

View reviewed changes

mikaelarguedas added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels May 24, 2018

mikaelarguedas requested a review from mjcarroll May 31, 2018 17:05

Update license to Apache 2.0

7990ce5

- Use config file with permissive license: http://www.gimpel.com/html/pub90/au-misra3.lnt

jpsamper2009 force-pushed the ament_pclint branch from 2f22615 to 7990ce5 Compare June 5, 2018 18:54

mjcarroll approved these changes Jun 14, 2018

View reviewed changes

mjcarroll merged commit 33f30be into ament:master Jun 14, 2018

mikaelarguedas removed the in review Waiting for review (Kanban column) label Jun 14, 2018

mjcarroll mentioned this pull request Jun 15, 2018

Revert "Add ament package for pclint" #100

Merged

mjcarroll mentioned this pull request Jun 18, 2018

ament_pclint: Fixing copyright and formatting issues. #101

Merged



		def find_executable(file_name, additional_paths=None):
		if sys.platform == 'linux':


		set(cmd "${ament_pclint_BIN}" ${ARG_UNPARSED_ARGUMENTS})

		get_property(INCLUDED_DIRS DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR} PROPERTY INCLUDE_DIRECTORIES)

Add ament package for pclint #98

Add ament package for pclint #98

Conversation

jpsamper2009 commented Apr 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirk-thomas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjcarroll Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpsamper2009 commented Apr 4, 2018

dirk-thomas commented Apr 4, 2018

jpsamper2009 commented Apr 16, 2018

mjcarroll commented Apr 16, 2018 • edited Loading

jpsamper2009 commented May 1, 2018

dirk-thomas commented May 1, 2018

mjcarroll commented May 1, 2018

jpsamper2009 commented May 1, 2018

mjcarroll commented May 17, 2018

dirk-thomas commented May 17, 2018

jpsamper2009 commented May 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpsamper2009 commented Jun 5, 2018

dirk-thomas commented Jun 14, 2018

mjcarroll commented Jun 15, 2018

serge-nikulin commented May 21, 2019

dirk-thomas commented May 21, 2019

mjcarroll Apr 16, 2018 •

edited

Loading

mjcarroll commented Apr 16, 2018 •

edited

Loading