cheshirekow / cmake_format

Source code formatter for cmake listfiles.
GNU General Public License v3.0
962 stars 105 forks source link

Specifying a config file in `pre-commit` hook GitHub actions #335

Closed jhlegarreta closed 5 months ago

jhlegarreta commented 11 months ago

I am trying to use cmake_format together with pre-commit and a GitHub Actions workdflow to check the format of some CMake code in the following repository: https://github.com/SlicerDMRI/SlicerDMRI

I have checked that the action works beautifully by default; I would be interested in using a specific configuration file. According to the pre-commit docs: https://pre-commit.com/#pre-commit-configyaml---hooks

I should use the args keyword to pass additional parameters to the hook in the .pre-commit-config.yaml file, i.e.

repos:
  - repo: https://github.com/cheshirekow/cmake-format-precommit
    rev: v0.6.10
    hooks:
      - id: cmake-format
        args: [-c Utilities/CMakeFormat/config.yaml]
      - id: cmake-lint
        args: [-c Utilities/CMakeFormat/config.yaml]

However, the GitHub Action says that it cannot find the config file at issue: https://github.com/SlicerDMRI/SlicerDMRI/actions/runs/6606209443/job/17942184338#step:4:116

It seems a path-related issue, maybe due to the current working directory the pre-commit hook is expecting, but it is unclear to me where the relative path is.

I would also expect the path to be valid when running pre-commit locally.

What am I missing?

Thanks for maintaining this tool.

yvesll commented 5 months ago

I meet the same issue recently, you can try this

args: ["-c",  "Utilities/CMakeFormat/config.yaml", "--"]

The last '--' is critical to let cmake-format find correct files

jhlegarreta commented 5 months ago

@yvesll thanks for chiming in.

Not sure this is having the desired effect ,though:

slicerdmri_cmake_format_default_config_reformat

slicerdmri_cmake_format_default__and_custom_config_vble_name_warning

There is only 2 differences between the default config file and my custom one (line_width: default 80 vs 120 in my config file; max_pargs_hwrap: default 6 vs 3 in my config file), I would expect the rest of the changes to be applied: e.g. the one corresponding to the positional argument horizontal wrap (max_pargs_hwrap) is set to 3 (default is 6), and thus the following change (proposed by the action when the config file is not used):

-#-----------------------------------------------------------------------------
-set(MODULE_PYTHON_SCRIPTS
-  ${MODULE_NAME}.py
-  )
+# -----------------------------------------------------------------------------
+set(MODULE_PYTHON_SCRIPTS ${MODULE_NAME}.py)

should be proposed, and it is not the case.

Or comments that are shorter than 120 chars, I would expect them to be reformatted (as the default does for 80), and it is not the case. Below what the action does when no config file is specified (default line_width 80):

slicerdmri_cmake_format_default_config_line_len_reformat

Maybe I'm missing something else.

[1] https://github.com/SlicerDMRI/SlicerDMRI/actions/runs/8925287318/job/24513616272?pr=186 [2] https://github.com/SlicerDMRI/SlicerDMRI/actions/runs/8924792791/job/24511923270?pr=186

jhlegarreta commented 5 months ago

When setting the line_width to 40 (smaller than the default), the action does indicate that lines are too long: https://github.com/SlicerDMRI/SlicerDMRI/actions/runs/8931355682/job/24533253783?pr=186

But it does not show the diff, as opposed to when the default config is used.

jhlegarreta commented 5 months ago

Using the "--check" or --in-place" flags show cmake-lint passing. I would assume the --check mode to mark the checker/linter to fail: https://github.com/cheshirekow/cmake_format/blob/eff5df1f41c665ea7cac799396042e4f406ef09a/cmakelang/doc/release_notes.rst#v062

Also, depending on whether or not the above flags are present, the errors shown/the formatting done is different, which does not make sense to me.

Not sure what I am missing.

yvesll commented 5 months ago

HI, @jhlegarreta, the only difference is the version of cmake-format. v0.6.10 also not work for me, maybe you can try v0.6.13

jhlegarreta commented 5 months ago

HI, @jhlegarreta, the only difference is the version of cmake-format. v0.6.10 also not work for me, maybe you can try v0.6.13

OK, looks like that is working. And I do not get the invalid variable name warning which did not seem to make sense.

So closing the issue. Thanks @yvesll :100:.