tools/nxstyle: Verify relative path in the file header

[gustavonihei]

Summary

Add new verification to nxstyle tool based on the NuttX Coding Standard.
From the 1.1 File Organization section:

• The relative path to the file from the top-level directory.

Limitations:

• For supporting input file provided with absolute path, nxstyle will search for the root folder assuming that it is named nuttx.
• When executed directly (e.g. not via checkpatch.sh), nxstyle will report a false positive in the following scenarios:
  • Path to the input file starts with ./
  • Path to the input file is not relative to the project root directory
• Currently it does not support the files from the apps/ folder (treated as false negatives).
• Failure to identify (i.e: false negative report) the scenario when one or more directories are omitted from the beginning of the relative path (.e.g.: "nxstyle.c" instead of "tools/nxstyle.c")

Impact

New verification rule for nxstyle, it will impact CI.

Testing

$ ./tools/checkpatch.sh -f sched/signal/sig_nanosleep.c
sched/signal/sig_nanosleep.c:2:1: error: Relative file path does not match actual file

[patacongo]

I wonder if we do not need some way to suppress this check? I often run nxstyle against files outside of the nuttx source tree especially when developing code or testing nxstyle.c.

[patacongo]

Doesn't this assume that the name of the root directory is nuttx/? That is not always the case.

[gustavonihei]

Yes, it is one of the limitations I listed in the description.
I haven't found a nice way of making nxstyle handle both relative and absolute file paths, since the program has no knowledge about the environment.
One way would be to provide the project root path via program arguments, but I think it is a bad solution.

[xiaoxiang781216]

how about this:

const char *tmp = strrstr(g_file_name, line + 2);
if (tmp && strlen(tmp) == strlen(line + 2) &&
    (tmp == g_file_name || *(tmp - 1) == '/'))

Yes, it can't catch the well-designed test case, but should enough to catch the most typo error.

[gustavonihei]

strrstr seems to be a C extension specific to Linux. It is not available in macOS.
We could implement it ourselves in nxstyle, though.

[xiaoxiang781216]

If so we can change to:

ssize_t base = strlen(g_file_name) - strlen(line + 2);
if (base >= 0 && (base == 0 || g_file_name[base - 1] == '/') &&
   strcmp(&g_file_name[base], line + 2) == 0

[xiaoxiang781216]

2. base seems to assume that the provided relative path in the header has at least has the same amount of characters. But this is usually not the case. If you remove a single char from the tools/nxstyle.c at the header, it will fail to identify the issue.

Why not? "g_file_name[base - 1] == '/"' should catch it.

[gustavonihei]

Suppose the relative file path in the comment is tools/nxstle.c
This condition will not be satisfied because g_file_name[base - 1] will point to the 't' character.

[xiaoxiang781216]

Suppose the relative file path in the comment is tools/nxstle.c

If user forget 'y' after 't', strcmp will report the mismatch.

This condition will not be satisfied because g_file_name[base - 1] will point to the 't' character.

If user typo 'ools/nxstyle.c', g_file_name[base] equals 'o' and g_file_name[base - 1] equals 't'. Then "g_file_name[base - 1] == '/"' will catch it.

[gustavonihei]

Ok, now I get. At first I thought your suggestion's objective was to identify the error, but now I understand that the condition you created will identify the correct file path. So the intended outcome is this:

    if (base >= 0 && (base == 0 || g_file_name[base - 1] == '/') &&
        strcmp(&g_file_name[base], &line[n + 2]) == 0)
        {
            printf("No error\n");
        }
    else
        {
            printf("Error detected\n");
        }

Yeah, this seems to work well. But without the knowledge of the project's root path, it will still fail to report an issue when one or more directories are missing in the beginning of the path:

https://godbolt.org/z/6Ec7oq

[xiaoxiang781216]

Yes, you are right: my algo can't catch all error, but the frequent error.

[gustavonihei]

I wonder if we do not need some way to suppress this check? I often run nxstyle against files outside of the nuttx source tree especially when developing code or testing nxstyle.c.

A solution for this scenario would be to decouple this check from nxstyle, letting this responsibility to the CI. But the downside is that we would lose the local checking prior to opening the PR.

[xiaoxiang781216]

We can provide an option to disable the check, so the developer can bypass the check case by case.

[xiaoxiang781216]

LGTM. Thanks @gustavonihei