Feature request: support `pre-commit` hook

[kevbroch]

What curious if the team would consider directly supporting a pre-commit hook based on https://pre-commit.com/#creating-new-hooks ?

[fangism]

Can you specify what you have in mind for pre-commit? the style-lint-clean? format-clean? both?

See also: https://github.com/google/verible/blob/master/verilog/tools/formatter/git-verilog_format.sh This runs the formatter on the current workspace, and formats changed lines only.

Related issues:

291 Lint changed lines only

292 Select base ancestor for comparison when incrementally formatting.

[fangism]

@eunchan @imphil @msfschaffner -- might be interested

[kevbroch]

It would be for both linting and formatting. My current usage model is focused on linting by manually calling verilog_lint on all files from the command line (for a subset of lint rules). I've set up a similar approach toverilog_fmt but some of the style changes are disagreed upon in the team so it's a WIP.

I did see and appreciate git-verilog_format.sh but I like the idea of keeping pre-commit in charge of the git specifics where possible.

I'm new to both verible and pre-commit but happy to help where needed.

[matlupi]

Working example

-   repo: local
    hooks:
    -   id: verible-format
        name: Verible verilog format
        entry: verible-verilog-format
        language: system
        types_or: [verilog, system-verilog]
        args: [--flagfile, .rules.verible_format]

You need to have a file called .rules.verible_format in your root directory with the rules you want to enable/disable. E.g.

+line-length=length:500

[cbachhuber]

While the working example is a good start, I'd prefer to have a hook that doesn't rely on a system-installed verible, but manages, as canonical for pre-commit hooks, the binary in a virtual environment. This facilitates distribution of pre-commit hooks on systems such as dev machines and CI containers.

I propose to do this similar to what has been done for clang-format:

• Provide a python wheel that allows you to pip install verible (see clang-format equivalent)
• Provide the hook that fetches this binary from pypi.

Both of these can be done in separate repos as in the clang-format example, or integrated into this repo. What do you think about that?

[cbachhuber]

Nobody interested? Since this repo has the Apache 2.0 license, I'll go ahead with these steps in my own repo, referencing your license, of course. I'll update about relevant progress here.

[hzeller]

If you have a solution, send a pull request.

[hzeller]

Note, for a while, the releases provide a statically linked set of binaries that should work on most Linux distributions out of the box. The github action that provides the linter uses that: https://github.com/chipsalliance/verible-linter-action (so, less need to set up a virtual environment)

[cbachhuber]

Nice, thank you for the info. I looked into distributing per PyPI:

• all widely used Python build/packaging systems such as scikit-build(-core) or setuptools want to build from source and not just 'pass on binaries'
• None of these tools natively supports Bazel (see also this blog post), they focus on CMake
• tensorflow still uses bazel and creates a python wheel, so there must be a way. I don't see how all these things play together in tensorflow, however.

I have a significant knowledge gap regarding Bazel and PyPI publishing, this is much more work than I anticipated, so I won't do it soon. Alternatively, we could publish the statically linked binaries in a dockerfile (this would also enable pre-commit.

I'm not sure that this is worth the effort, though, given that there are GitHub actions for linting and formatting (thanks for sharing!). However, those actions need to upload checked files to reviewdog, which might not be acceptable for private projects. Thus, there's still value in supporting the pre-commit hook, I think. What's your opinion?

[hzeller]

I have not much of an opinion w.r.t. these hooks.

The workflows I am in typically either have local scripts that help clean up code bases or they rely on the CI to do these things. Some sort of project-maintained run-this-before-submit.sh script can do whatever is needed to verify that something is fit for submit, and it is much more straightforward, easier to understand and extend than some 'magic' behind walls of containers, magic Python scripts and other things that might be hard to debug when it breaks.

It sounds like it might be more hassle than actually worth, but I might be wrong and if such thing existed there might be many folks using it (but it sounds like it should be maintained separately independent of this repo; like for the lint actions that are in a separate repo, we can add a link to your pre-commit implementation).

[cbachhuber]

For reference, I ended up downloading the verible binary in my github action

jobs:
  pre-commit:
    runs-on: ubuntu-20.04
    steps:
    - uses: actions/checkout@v3
    - name: Install verible
      run: |
        wget https://github.com/chipsalliance/verible/releases/download/v0.0-3724-gdec56671/verible-v0.0-3724-gdec56671-linux-static-x86_64.tar.gz
        tar -xvzf verible-v0.0-3724-gdec56671-linux-static-x86_64.tar.gz
        export PATH=$PATH:$PWD/verible-v0.0-3724-gdec56671/bin
        echo "PATH=$PATH" >> $GITHUB_ENV
      shell: bash
    - run: python -m pip install pre-commit
      shell: bash
    - run: python -m pip freeze --local
      shell: bash
    - uses: actions/cache@v4
      with:
        path: ~/.cache/pre-commit
        key: pre-commit-3|${{ hashFiles('.pre-commit-config.yaml') }}
    - run: pre-commit run --show-diff-on-failure --color=always --all-files
      shell: bash

The latter part of this action is just the expanded pre-commit github action. If I didn't expand it, it wouldn't consume the PATH env var.

And combine this with the pre-commit hook suggested earlier in this issue.

repos:
  - repo: local
    hooks:
      - id: verible-format
        name: Verible verilog format
        entry: verible-verilog-format
        language: system  # this is important: it means that pre-commit assumes that the binary is available in the system. That's why we installed in the pre-commit action
        types_or: [verilog, system-verilog]
        args: [--inplace]