newly created package fails colcon test

[christianrauch]

Bug report

Creating a new node with ros2 creates a package that fails colcon test.

Required Info:

• Operating System: Ubuntu 20.04
• Installation type: binaries
• Version or commit hash: 0.9.10

Steps to reproduce issue

1. create a new node: ros2 pkg create my_package --build-type ament_cmake --dependencies rclcpp --node-name my_node
2. run tests: colcon build , colcon test

Expected behavior

A newly created package should follow ROS2 standards and not fail its own tests.

Actual behavior

colcon test fails:

$ colcon test
Starting >>> my_package
--- stderr: my_package                   
Errors while running CTest
---
Finished <<< my_package [2.70s]	[ with test failures ]

$ colcon test-result --verbose
build/my_package/Testing/20211128-1821/Test.xml: 6 tests, 0 errors, 2 failures, 0 skipped
- copyright
  <<< failure message
    -- run_test.py: invoking following command in '/tmp/ws/src/my_package':
     - /opt/ros/foxy/bin/ament_copyright --xunit-file /tmp/ws/build/my_package/test_results/my_package/copyright.xunit.xml
    src/my_node.cpp: could not find copyright notice
    1 errors, checked 1 files
    -- run_test.py: return code 1
    -- run_test.py: verify result file '/tmp/ws/build/my_package/test_results/my_package/copyright.xunit.xml'
  >>>
- cpplint
  <<< failure message
    -- run_test.py: invoking following command in '/tmp/ws/src/my_package':
     - /opt/ros/foxy/bin/ament_cpplint --xunit-file /tmp/ws/build/my_package/test_results/my_package/cpplint.xunit.xml
    /tmp/ws/src/my_package/src/my_node.cpp:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
    Category 'legal/copyright' errors found: 1
    Total errors found: 1
    Using '--root=/tmp/ws/src/my_package/src' argument
    
    Done processing /tmp/ws/src/my_package/src/my_node.cpp
    
    -- run_test.py: return code 1
    -- run_test.py: verify result file '/tmp/ws/build/my_package/test_results/my_package/cpplint.xunit.xml'
  >>>
build/my_package/test_results/my_package/copyright.xunit.xml: 1 test, 0 errors, 1 failure, 0 skipped
- my_package.copyright src/my_node.cpp
  <<< failure message
    could not find copyright notice
  >>>
build/my_package/test_results/my_package/cpplint.xunit.xml: 1 test, 0 errors, 1 failure, 0 skipped
- my_package.cpplint legal/copyright [5] (/tmp/ws/src/my_package/src/my_node.cpp:0)
  <<< failure message
    No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"
  >>>

Summary: 12 tests, 0 errors, 4 failures, 0 skipped

[clalancette]

So looking at the failures, they both revolve around the copyright. This makes some sense, as the copyright wasn't specified in the initial ros2 pkg create. That said, adding the --license flag doesn't improve the situation, because the copyright lines are not added to the top of the skeleton main.

Unfortunately, we don't have all of the information required in order to properly fill in that copyright header. In order to do that, we need both the copyright header text and the copyright holder. If the user specifies --license, and it is one of the licenses we know, then we know the copyright header text. But we currently have no way to get the copyright holder.

At the end of the day, this is a tradeoff. If we want to be rigorous and ensure that we'll pass the tests, then we'd need to have a required flag for the license (which we already have), and a required flag for the copyright holder (which we would have to add). This would go further towards our goal of making all packages have some license, at the expense of making this utility more pedantic and more difficult to use. The other way you could argue this is that the license is something that we want the user to examine and make deliberate choices about, so having it fail the tests out-of-the-box is the way it should be.

I'm on the fence about which way to go here. @ros2/team any opinions?

[christianrauch]

But we currently have no way to get the copyright holder.

What about just using the MAINTAINER_NAME? When you create the package, this is most likely the person that is going to write the initial code.

required flag for the copyright holder

In case no copyright holder is specified or it is not taken from the maintainer, I would just deactivate the test in the CMake (set(ament_cmake_copyright_FOUND TRUE)).

This would go further towards our goal of making all packages have some license

I wouldn't mind if package creators are forced to choose a licence when publishing code online. This makes it much easier to reuse packages. But this makes it also a bit cumbersome for users that just want to create a package for personal use.

In summary, I think that those values should either be determined automatically from the provided parameters, or the tests should be deactivated if required values cannot be determined.

[aprotyas]

One way to address the copyright issue is to disable the copyright test by default (uncomment line 82 in the linked snippet), allowing the package author to enable it later just by commenting a single line in the CMakeLists.txt file.

ros2cli/ros2pkg/ros2pkg/resource/ament_cmake/CMakeLists.txt.em

Lines 80 to 82 in 37fea06

	# the following line skips the linter which checks for copyrights
	# uncomment the line when a copyright and license is not present in all source files
	#set(ament_cmake_copyright_FOUND TRUE)

That would still keep the tool relatively easy to use.

For "smart" default behavior, maybe the set(ament_cmake_copyright_FOUND TRUE) command can be conditionally commented out in the CMakeLists.txt template if the user specifies a valid/known --license option?

[christianrauch]

I noticed that even after disabling ament_cmake_copyright by commenting that line, colcon test-result --verbose will still show the issue. It's just that colcon test will not fail.

[clalancette]

What about just using the MAINTAINER_NAME? When you create the package, this is most likely the person that is going to write the initial code.

We could do that, but I think there are no instances in the core codebase where the maintainer name is the same as the copyright holder name. That is, the maintainer is generally a person, while the copyright holder is generally an organization. So while we could rely on this, it will probably be wrong (and will pass the tests, meaning that the developer may overlook changing it).

One way to address the copyright issue is to disable the copyright test by default (uncomment line 82 in the linked snippet), allowing the package author to enable it later just by commenting a single line in the CMakeLists.txt file.

I would be onboard with that. It doesn't fail the tests out-of-the-box, but it is at least a marker that the developer should do something about it.

[jacobperron]

I noticed that even after disabling ament_cmake_copyright by commenting that line, colcon test-result --verbose will still show the issue. It's just that colcon test will not fail.

@christianrauch This could be due to a left-over test result. Try again in a clean workspace (e.g. remove the build, install and log directories).

---

+1 for disabling the test by default.

We can still pester the user with comments in the code to encourage them to add a license/test.