Switch cpplint to new repository

[jwnimmer-tri]

See google/styleguide#837. The upstream styleguide no longer provides cpplint. We should switch our external to point to https://github.com/cpplint/cpplint instead. That will mean splitting out half of the logic from https://github.com/RobotLocomotion/drake/tree/master/tools/workspace/styleguide into a new external at drake/tools/workspace/cpplint_internal.

[Aiden2244]

@jwnimmer-tri Hey Jeremy, this is Aiden. I recently started development on this project at Kitware and was asked to review this issue. Below is a summary of my findings regarding cpplint version conflicts between the upstream (cpplint/cpplint) and Drake's internally maintained fork (RobotLocomotion/styleguide). The gist is that the latest stable release of cpplint from the upstream causes a significant portion of Drake's cpplint tests to fail locally without introducing additional filters. I would greatly appreciate your thoughts on how to proceed from here. Thank you for your attention!

cpplint upgrade docs

Switching from the fork of cpplint defined in the repo RobotLogisticts/styleguide to the new external target at cpplint/cpplint caused a number of tests to fail in the old test suite, all relating to cpplint. Specifically, of 8772 local tests, 1666 fail, all relating to cpplint. I have included a sample log for reference:

exec ${PAGER:-/usr/bin/less} "$0" || exit 1
Executing tests from //systems/analysis:py/monte_carlo_test_cpplint
-----------------------------------------------------------------------------
systems/analysis/test/monte_carlo_test.cc:24:  Do not indent within a namespace.  [whitespace/indent_namespace] [4]
systems/analysis/test/monte_carlo_test.cc:25:  Do not indent within a namespace.  [whitespace/indent_namespace] [4]
systems/analysis/test/monte_carlo_test.cc:48:  Do not indent within a namespace.  [whitespace/indent_namespace] [4]
systems/analysis/test/monte_carlo_test.cc:189:  Add #include <unordered_set> for unordered_set<>  [build/include_what_you_use] [4]
systems/analysis/test/monte_carlo_test.cc:253:  Add #include <memory> for make_unique<>  [build/include_what_you_use] [4]
systems/analysis/test/monte_carlo_test.cc:253:  Add #include <utility> for move  [build/include_what_you_use] [4]
Done processing systems/analysis/test/monte_carlo_test.cc
Total errors found: 6

Upon investigating this issue, I noticed that the tests that were failing were generally of the same few cpplint error categories. To get tests to pass, I incrementally filtered these error categories out until all tests were failing. In all, there were 9 problematic error categories, which I noticed fell into one of two classes. I cross-referenced how each of the problematic error categories were used in both the latest version of our internally maintained fork of cpplint and the latest stable release from the upstream. The first 4 listed below are error categories which are defined in the latest stable release of cpplint from the cpplint repository but are not defined in the fork of cpplint that drake was previously using. The remaining 5 categories were defined in both the legacy fork and the latest stable release from cpplint, but the latter repository contained updated functionality which increased the number of positives detected. The error categories and their respective groupings are provided below:

# These conditions were causing tests to fail after changing the cpplint
# target from RobotLogistics/styleguide to cpplint/cpplint. Notably, these
# filters were NOT listed as error categories in the legacy target for cpplint
# (RobotLocomotion/styleguide) and were presumably added in later updates.
build/c++17
build/namespaces_headers
build/namespaces_literals
whitespace/indent_namespace

# These error categories ARE defined in the legacy target, yet only began
# causing problems when the cpplint target was switched to (cpplint/cpplint).
# Generally, the updated version of cpplint caused these filters to
# become more strict and trip up the test suite more often.
build/include_what_you_use
readability/casting
runtime/references
runtime/explicit 
whitespace/newline

To circumvent this issue, I opted to use the build of cpplint from the upstream repository cpplint/cpplint which had the closest release date to the version of cpplint previously being used, instead of the latest stable release of cpplint. This successfully satisfied the existing test suite while simultaneously moving away from the internally maintained fork of cpplint. It is probably to separate the upgrading of cpplint to the latest stable release into its own PR.

Update 3/3/2025

The commit hash from the cpplint/cpplint repository which most closely aligns with the previous version being used (henceforth referred to as the RobotLocomotion fork) is 554223dc5432f9bd67951cde662f7a023c512dec .

Given this observation, I noted three things which are interesting and relevant to this issue:

1. The aforementioned commit is from more than 10 years ago.
2. At this point in history, the cpplint/cpplint repository keeps the cpplint.py file defined within a subdirectory called cpplint/ . This directory structure is the SAME as the one present in the current version of the RobotLocomotion fork, but is DIFFERENT from the current version of cpplint.py in the cpplint/cpplint repository (the file is now defined at the root of the file). The cpplint/cpplint maintainers changed the directory structure of this repository on December 14th, 2015 to the structure it is in today.
3. Evidently, there is no point in the history of the cpplint/cpplint repository where the version of cpplint.py is exactly the same as the current version of that file present in the RobotLocomotion fork.

This indicates to me that the fork of cpplint being used in Drake prior to my changes was based on an older model of the upstream cpplint repository, with considerable independent and divergent development occurring since that time. As the forked version of cpplint still contains the cpplint subdirectory, I believe that December 2015 is a plausible estimate for when these two versions diverged. Given this, there unfortunately does not appear to be an upstream 1:1 replacement for the version of cpplint used in Drake.

Since there is no drag-and-drop replacement for the fork of cpplint Drake is currently using in the upstream, addressing this issue means making a compromise. We must either be ok with temporarily changing the specificity of cpplint as it applies to this project (allowing it to be slightly more or less strict than before), attempt to use a much older version of cpplint from the upstream, or address the 1600 or so cpplint errors present in a different way.

Here are the ways that I could proceed on this issue from here:

First, I could go with the approach I initially started with, using the newest stable release of cpplint from about a month ago and filtering out any of the error categories that are causing a problem. I could then raise as a follow up that these error categories need to be adjusted for incrementally in future PRs.

Second, I could try and use the commit of cpplint mentioned above. There are a number of issues with this approach that make this undesirable in my opinion. Not only would the version of cpplint be reverted to something much older, it would require me to re-refactor the bazel build files and usages of cpplint due to the change in where cpplint.py is located (i.e. within a subdirectory as opposed to at the root of the repository).

Finally, I could investigate this issue further and see if there is a way to use the updated cpplint repository and address the failed test cases in the codebase (without having to manually address 1600 local build errors).

In my view, I believe the most sensible approach is the first one. It would require that the individual cpplint errors be addressed incrementally in the future, but re-refactoring the code to work with an outdated version of the upstream or addressing these issues as part of this PR both seem like too much work for one PR.

[jwnimmer-tri]

We should target the newer / newest cpplint, not try to use an old one.

In terms of the new warnings:

• if they are true positives (real problems), then we should fix them;
• if they are false positives (not real problems), then we should figure out how to silence them, either by bug-fixing upstream cpplint or changing our configuration;

In order for us to weigh in on both of those points, we need to see and run your draft. Please open a Draft PR and label it "do not review", containing the repository rule change that points Drake to the new cpplint repo at the newest (or at least very recent) version. Then other people can reproduce your experiment, and offer more specific advice.

[Aiden2244]

Understood. I will open that request shortly.