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.

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

[FEATURE] Add support for better pre-commit #12912

Closed
1 task done
fdcavalcanti opened this issue Aug 14, 2024 · 2 comments · Fixed by #13021
Closed
1 task done

[FEATURE] Add support for better pre-commit #12912

fdcavalcanti opened this issue Aug 14, 2024 · 2 comments · Fixed by #13021
Labels
Type: Enhancement New feature or request

Comments

@fdcavalcanti
Copy link
Contributor

fdcavalcanti commented Aug 14, 2024

Hi folks,

I would like to propose the addition of pre-commit support on NuttX.
Yes, I'm aware there's a pre-commit script on tools. I do not want to replace it.

Today, we need to manually run checkpatch.sh or add the available pre-commit script to our git hook. There's another option that should be more user friendly and most important, adds more linter options.

This is the tool: https://pre-commit.com/
I've used it before with massive repositories, different languages and it has always worked great.

Describe the solution you'd like

Additions to the repository
A single addition would be necessary, which is a file called .pre-commit-config.yamlon the root directory (could go on nuttx-apps repository as well).

Maintenance
Little maintenance required. Simply add or remove the linter scripts from the config file.

Are we losing checkpatch.sh?
No, as a first step, we could add checkpatch.sh to the pre-commit tool so we don't need to move all linters that already working from checkpatch.shto this config file (although it is possible).
Also, we don't interfere with CI.

Is it mandatory for me to use it?
No, use of this tool is not mandatory. You can simply ignore it if you want.

Config file example:
This is the config file I'm running right now. It is located in the root directory under

$ cat .pre-commit-config.yaml 
repos:
-   repo: https://github.com/pre-commit/pre-commit-hooks
    rev: v4.6.0
    hooks:
    -   id: end-of-file-fixer
    -   id: trailing-whitespace
    -   id: check-added-large-files
-   repo: local
    hooks:
    -   id: nxstyle
        name: nxstyle
        language: script
        entry: ./tools/checkpatch.sh -f

You can see that I'm running checkpatch as a script language. checkpatch itself runs black and isort for Python files, those can also be added to this config file.
It's also possible to simply add nxstyle instead of the checkpatchscript.

How to enable it
Install the pre-commit tool on your machine:
pip install pre-commit.
You need pip because this package is Python based.

Note about Ubuntu:
If using Ubuntu 24+, maybe you will need to install with apt because Python packages cannot be installed directly with pip anymore. In this case:
sudo apt install pre-commit should suffice.

After installing, enter the NuttX directory and simply run pre-commit install
This should install the hooks that are defined on the config file:

fdcavalcanti@espubuntu:~/nuttxspace/nuttx$ pre-commit install
pre-commit installed at .git/hooks/pre-commit

Usage
Simple commit example: I've added a white space at the end of a line in esp32s2_wlan.c and messed with indentation on a if statement. You can see it fixed the whitespace issue was automatically fixed and now I should simply git add this file again, after correcting the nxstyle complaints.

fdcavalcanti@espubuntu:~/nuttxspace/nuttx$ git commit -m "Testing pre-commit"
fix end of files.........................................................Passed
trim trailing whitespace.................................................Failed
- hook id: trailing-whitespace
- exit code: 1
- files were modified by this hook

Fixing arch/xtensa/src/esp32s2/esp32s2_wlan.c

check for added large files..............................................Passed
nxstyle..................................................................Failed
- hook id: nxstyle
- exit code: 1

/home/fdcavalcanti/nuttxspace/nuttx/arch/xtensa/src/esp32s2/esp32s2_wlan.c:737:11: error: Bad right brace alignment

After correction, the commit is done.

fdcavalcanti@espubuntu:~/nuttxspace/nuttx$ git commit -m "Testing pre-commit"
fix end of files.........................................................Passed
trim trailing whitespace.................................................Passed
check for added large files..............................................Passed
nxstyle..................................................................Passed
[feature/esp32s2_wifi 8394e9f3cf] Testing pre-commit
 1 file changed, 1 insertion(+)

I believe there's room to improve our linting, use other hooks and I this can be also extended to the CI.
What do you guys think?

Describe alternatives you've considered

No response

Verification

  • I have verified before submitting the report.
@fdcavalcanti fdcavalcanti added the Type: Enhancement New feature or request label Aug 14, 2024
@xiaoxiang781216
Copy link
Contributor

nice idea, @fdcavalcanti !

@fdcavalcanti
Copy link
Contributor Author

So, I'll send the PR soon with the features I mentioned previously.

I tried to expand the config file to do everything checkpatch does. Worked fine except for nxstyle:

  • pre-commit passes a relative path from root and since nxstyle is inside tools, it can't work itself out.
  • nxstyle must be built before running, which goes against the standard tools used with pre-commit.

I wonder if some other generic linting tool could be configured to work just as nxstyle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants