[echoix]

Supercedes #7663
Closes #7346
Closes #7686

Adds gersemi formatter in pre-commit. Removes an unused cmake file as per #7686, caught through the warnings the linter caused.

I chose to keep the tool's default indentation of 4 spaces; also added to .editorconfig so that when we type, most editors will already make you indent correctly. Either 4 or 2 space indentation had the same amount of changed files, in the same file both were used. After taking a close look at both choices, the default still made more sense. It is a one-time change, then automatic.

I had to replace the previous PR because of the difficulty of merging correctly previous work and the new one made during the sprint, and too hard to untangle with rebases.

[echoix]

Ideally we should merge this before other CMake files get changed, or in the other cases, update back this PR and apply formatting to new files/changed files

[nilason]

It would be good to add custom formatting for at least the most commonly used functions/macros (e.g. build_library_in_subdir, build_program_in_subdir, build_program, build_module, build_script_in_subdir).

[echoix]

It would be good to add custom formatting for at least the most commonly used functions/macros (e.g. build_library_in_subdir, build_program_in_subdir, build_program, build_module, build_script_in_subdir).

Isn’t it already doing that, generating a custom formatter since I specified the folder of the definitions with --definitions? When it wasn’t there, when I enabled the extra warnings, I was able to see that there was unknown functions, but configured like it is now, it understands it.

There’s a probability that we don’t respect the « deal » (from https://github.com/BlankSpruce/gersemi#lets-make-a-deal) , where the function isn’t close enough to a straightforward function that could be understood easily enough, such as you find the output not optimal, but I don’t think it is that worse. We at least have a working tool, that is not dead and not supporting newer CMake versions (like the other possible tool that was already abandoned at the time of the last research).

[nilason]

It would be good to add custom formatting for at least the most commonly used functions/macros (e.g. build_library_in_subdir, build_program_in_subdir, build_program, build_module, build_script_in_subdir).

Isn’t it already doing that, generating a custom formatter since I specified the folder of the definitions with --definitions? When it wasn’t there, when I enabled the extra warnings, I was able to see that there was unknown functions, but configured like it is now, it understands it.

Now, I see build_library_in_subdir, build_program_in_subdir, build_program are macros calling the main functionbuild_module either directly or indirectly. Formatting the use of build_module is correct (as in the rare example of lib/proj/CMakeLists.txt), but not the macros which are used extensively in the CMakeLists files.

[echoix]

Now, I see build_library_in_subdir, build_program_in_subdir, build_program are macros calling the main functionbuild_module either directly or indirectly. Formatting the use of build_module is correct (as in the rare example of lib/proj/CMakeLists.txt), but not the macros which are used extensively in the CMakeLists files.

Ok, and is this a deal-breaker, a blocker, for you now? Can we start with this for now anyways?

[echoix]

I’ll keep the PR updated with new CMake files incoming for now

[nilason]

Now, I see build_library_in_subdir, build_program_in_subdir, build_program are macros calling the main functionbuild_module either directly or indirectly. Formatting the use of build_module is correct (as in the rare example of lib/proj/CMakeLists.txt), but not the macros which are used extensively in the CMakeLists files.

Ok, and is this a deal-breaker, a blocker, for you now? Can we start with this for now anyways?

Not a blocker for me. I would like to remove/replace the *__in_subdir type of macros with explicit call to the functionbuild_module (and not grep-ing the source files) anyway. build_program is easy to replace.

[echoix]

I found a way to greatly improve the formatting, by using a stub definition that it understands (it is also a solution mentioned in their readme).

I would like to know, does the order of the elements in DEPENDS or other multiValueArgs matter? If not, I could make it be sorted, or even better, sorted+unique.

For example:

[nilason]

I found a way to greatly improve the formatting, by using a stub definition that it understands (it is also a solution mentioned in their readme).

That is great!

I would like to know, does the order of the elements in DEPENDS or other multiValueArgs matter? If not, I could make it be sorted, or even better, sorted+unique.

Order does not matter, sorted+unique would be a fine addition!

[echoix]

@nilason I pushed the changes, but do you have a better preference on how should the arguments be presented (with the hints)? Currently, I set all of the multiValueArgs keywords to sort+unique. The available options are pairs, command_line, sort, unique, or sort+unique, as noted here: https://github.com/BlankSpruce/gersemi/tree/master#gersemi-hints

If you have a suggestion, post either here or in a review comment. I don't see a case where we would use pairs or command_line (in these). Maybe you do.

[nilason]

The multi value args from build_module are:

"SOURCES;INCLUDES;DEPENDS;OPTIONAL_DEPENDS;PRIMARY_DEPENDS;DEFS;HEADERS;TEST_SOURCES"

which, as far as I can tell, can be hinted as sort+unique.

[echoix]

The multi value args from build_module are:

"SOURCES;INCLUDES;DEPENDS;OPTIONAL_DEPENDS;PRIMARY_DEPENDS;DEFS;HEADERS;TEST_SOURCES"

which, as far as I can tell, can be hinted as sort+unique.

The stubs all use the exact same body, and is the same as the build_module definition (only the variables needed to call cmake_parse_arguments() and cmake_parse_arguments() itself.

Note that I changed the build_module to look like all the docs, like https://github.com/BlankSpruce/gersemi/tree/master#how-to-format-custom-commands-for-which-path-to-definition-cant-be-guaranteed-to-be-stable-eg-external-dependencies-not-managed-by-cmake, or the more or less recent examples in the https://cmake.org/cmake/help/v4.4/command/cmake_parse_arguments.html#cmake-parse-arguments docs (it wasn't there when we developped it).

It follows this pattern:

function(try_to_win_best_picture_academy_award)
    # gersemi: hints { CAST: pairs, SUMMARY: command_line }
    set(options FOREIGN_LANGUAGE)
    set(oneValueArgs GENRE YEAR)
    set(multiValueArgs DIRECTORS CAST SUMMARY)

    cmake_parse_arguments(_ "${options}" "${oneValueArgs}" "${multiValueArgs}" ${ARGN})
endfunction()

[echoix]

I enabled auto-merge so it could go when it gets approved; each other merged PR that changes a CMake file immediately conflicts and I end up needing to rebase first.

[nilason]

Thanks for taking care of my issues! Now it looks fine to me. However, I'll leave to you guys at the sprint to decide when to merge this.

[echoix]

Well, we are still out, but if you don’t mind updating the PRs, the PR that fixed all windows CI issues for the whole day was finally auto merged 1h30 ago. So all PRs need that fix updated to their branch to pass.

[nilason]

Well, we are still out, but if you don’t mind updating the PRs, the PR that fixed all windows CI issues for the whole day was finally auto merged 1h30 ago. So all PRs need that fix updated to their branch to pass.

I updated a bunch of them.

[echoix]

Good!

[echoix]

Thanks for taking care of my issues! Now it looks fine to me. However, I'll leave to you guys at the sprint to decide when to merge this.

Did you approve the PR or are just positive about it now?

[echoix]

Oh, I’ll need to do another round of rebase, it wasn’t merged in time.