Skip to content

Conversation

@Bill-hbrhbr
Copy link
Collaborator

@Bill-hbrhbr Bill-hbrhbr commented Jan 24, 2025

Description

  • Adds the lint:cmake-check and lint:cmake-fix tasks for formatting following files:
    • CMakeLists.txt
    • Auxiliary CMake script files with .cmake suffix
    • Template CMake files with .cmake.in suffix
  • Adds a boilerplate CMake project that contains all 3 file types above so that we can validate the new task workflows.

The primary objective is to ensure that the lint:cmake-check and lint:cmake-fix workflows are as general and portable as possible. Other repositories should be able to copy this file and use it with little to no modification.

The CMake linting tasks in this PR follow the same approach as the one used in the initial lint taskfile of Spider, rather than the method adopted in the current version.

The initial version:

  cmake:
    internal: true
    requires:
      vars: ["FLAGS"]
    cmd: |-
      . "{{.G_LINT_VENV_DIR}}/bin/activate"
      find . \
        -path ./build -prune \
        -o -name CMakeLists.txt \
        -print0 | \
          xargs -0 --no-run-if-empty gersemi {{.FLAGS}}

The current version:

  cmake:
    internal: true
    requires:
      vars: ["FLAGS"]
    sources:
      - "CMakeLists.txt"
      - "src/spider/CMakeLists.txt"
      - "tests/CMakeLists.txt"
      - "cmake/Modules/*.cmake"
    cmds:
      - for: "sources"
        cmd: |-
          . "{{.G_LINT_VENV_DIR}}/bin/activate"
          gersemi {{.FLAGS}} {{.ITEM}}

The reason why we pick the first version:

  • The first version only runs gersemi once on the entire batch of files to be linted, while the second version runs gersemi once on every single file.
    • The former provides a better output format.
    • The latter task stops on the first ill-formatted file. Making users repeatedly run lint tasks after fixing each file is tedious.
  • The second version puts the sources section in the internal helper function. This is generally not a good practice since it might prevent other tasks that call this internal task from running.

On top of the first version, this PR makes several improvements:

  • Linting not only applies to CMakeLists.txt, but also to other types of CMake files, as explained above.
  • Moving the sources section to the top level ensures that top-level tasks operate independently and do not interfere with each other when determining whether execution is necessary.
  • Adds a colored diff output to the underlying operation of lint:cmake-check. The error code will be returned as normal when files are ill-formatted.
  • Adding a new library to make diff colorable.

Validation performed

  • New task workflows succeed in detecting and fixing CMake files.

@Bill-hbrhbr Bill-hbrhbr requested a review from davidlion January 24, 2025 12:20
@Bill-hbrhbr
Copy link
Collaborator Author

Bill-hbrhbr commented Jan 24, 2025

@davidlion Hey David you might want to ask Kirk on some things I've done here, since they are not really conforming to the version used by the latest commits in spider.

Copy link
Member

@davidlion davidlion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets just drop the CMake/ directory for now. I feel like those files have too little context atm.

@davidlion davidlion merged commit 3b6997f into y-scope:main Jan 27, 2025
2 checks passed
@Bill-hbrhbr Bill-hbrhbr deleted the add-cmake-lint branch February 3, 2025 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants