BlankSpruce / gersemi

A formatter to make your CMake code the real treasure
Mozilla Public License 2.0
156 stars 8 forks source link

Indentation for strings fails #30

Closed remod closed 4 months ago

remod commented 4 months ago

Thank you for this tool! Finally no manual CMakeLists.txt formatting anymore :)

I have a scenario where the formatter seems to fail:

    rosidl_generate_interfaces(${PROJECT_NAME} 
      "msg/Duration.msg"
        "msg/DynamicTimePoint.msg"
      "msg/TimeSourceType.msg"
      "msg/SteadyTimePoint.msg"      "msg/SystemTimePoint.msg"
    )

I would have expected that it a) fixes the indentation and b) puts every item on a new line. I've read the Let's make a deal section but I am under the impression that this should work nonetheless.

BlankSpruce commented 4 months ago

Technically ${PROJECT_NAME} could be a keyword, unusually named one but a keyword nonetheless. I've dealt with this once in another issue hence I'm thinking about this extraordinary situation. Of course you could move the goalpost and ask: how about this code?

    rosidl_generate_interfaces("${PROJECT_NAME}"
      "msg/Duration.msg"
        "msg/DynamicTimePoint.msg"
      "msg/TimeSourceType.msg"
      "msg/SteadyTimePoint.msg"      "msg/SystemTimePoint.msg"
    )

I guess quoted arguments in any command shouldn't constitute a keyword so I guess it'd be doable to introduce some formatting. I'm afraid though there's a risk of unobvious behaviour that is only technically consistent. It probably wouldn't be obvious when code above is "well-formatted" but the code like this isn't because gersemi doesn't know whether msg/TimeSourceType.msg is a keyword or not:

    rosidl_generate_interfaces(${PROJECT_NAME}
      msg/Duration.msg
        msg/DynamicTimePoint.msg
      msg/TimeSourceType.msg
      msg/SteadyTimePoint.msg      msg/SystemTimePoint.msg
    )

Such an unobvious behaviour can easily be confused with seeming inconsistency and I don't like that.

Nevertheless, I'll think about your case because everybody likes convenience.

BlankSpruce commented 4 months ago

I guess quoted arguments in any command shouldn't constitute a keyword

Nevermind that. This is legal in official commands. :facepalm:

#!/bin/bash

function log() {
    echo "[reproduction]" $@
}

log "Create dummy project directory"
mkdir -p /tmp/gersemi-30-repro
cd /tmp/gersemi-30-repro
rm -rf *

log "Create CMakeLists.txt"
cat<<EOF > CMakeLists.txt
project(foobar)
cmake_minimum_required(VERSION 3.25)

add_library(foolib foolib.cpp)

add_executable(foo foo.cpp)
target_link_libraries(
    foo
    "PRIVATE" # quoted PRIVATE for the lulz
    foolib
)
EOF

log "Create C++ files"
cat<<EOF > foo.cpp
#include "foolib.hpp"

int main() { hello(); }
EOF

cat<<EOF > foolib.hpp
void hello();
EOF

cat<<EOF > foolib.cpp
#include <iostream>

void hello() { std::cout << "Hello darkness my old friend\n"; }
EOF

log "Run cmake"
mkdir -p build
cd build
cmake ..

log "Build foo"
make foo

log "Check the output"
./foo

image

BlankSpruce commented 4 months ago

This code will be referred below:

    rosidl_generate_interfaces(${PROJECT_NAME} 
      "msg/Duration.msg"
        "msg/DynamicTimePoint.msg"
      "msg/TimeSourceType.msg"
      "msg/SteadyTimePoint.msg"      "msg/SystemTimePoint.msg"
    )

1) I could issue warnings about unknown commands that would help identifying lack of proper --definition or definition (.gersemirc). In this particular case (and I'm guessing since I'm not familiar with the codebase) this is the definition of macro in the example and as I've checked with 0.13.5 it honors The Deal. This would be something along the lines:

$ gersemi --check ./src/cmake
Warning: unknown command 'rosidl_generate_interfaces', consider providing source file or stub file through 'definitions' argument.
/tmp/my-project/src/cmake/foo.cmake would be reformatted
/tmp/my-project/src/cmake/bar.cmake would be reformatted

1a) I could even go plus ultra and issue a hint about possible source of that definition in case definition file is one of the files to be formatted/checked. Something like:

$ gersemi --check ./src/cmake
Warning: unknown command 'rosidl_generate_interfaces', consider providing source file or stub file through 'definitions' argument. Potential definition found in file: ./src/cmake/rosidl_utilities.cmake (path relative to currently used .gersemirc)
/tmp/my-project/src/cmake/foo.cmake would be reformatted
/tmp/my-project/src/cmake/bar.cmake would be reformatted

but no hint when:

$ gersemi --check ./src/cmake/foo.cmake ./src/cmake/bar.cmake
Warning: unknown command 'rosidl_generate_interfaces', consider providing source file or stub file through 'definitions' argument.
/tmp/my-project/src/cmake/foo.cmake would be reformatted
/tmp/my-project/src/cmake/bar.cmake would be reformatted

2) Actually do the minimum of upholding my end of The Deal and offer this formatting:

# assume indent 4
rosidl_generate_interfaces(${PROJECT_NAME} 
    "msg/Duration.msg"
      "msg/DynamicTimePoint.msg"
    "msg/TimeSourceType.msg"
    "msg/SteadyTimePoint.msg"      "msg/SystemTimePoint.msg"
)
# assume indent 13
rosidl_generate_interfaces(${PROJECT_NAME} 
             "msg/Duration.msg"
               "msg/DynamicTimePoint.msg"
             "msg/TimeSourceType.msg"
             "msg/SteadyTimePoint.msg"      "msg/SystemTimePoint.msg"
)

No line breaking and no tidying up Duration.msg and DynamicTimePoint.msg to the same visual line because for all I know it could be intentional. Given yet another quirk of CMake language as presented in previous comment I can't even reliably consider "quoted arguments aren't keywords". :(

This is as far as I'm willing to cast spells enhancing silicon rock to do the thing. Something better will require probably more magic or AI that's tuned to this kind of stuff.

All of these options offer something but they are progressively more demanding on how much time I would have to spent. Ideally I would like to have implemented all of them but I don't have that much time currently. Here's the question: what's the acceptable minimum for you @remod?

remod commented 4 months ago

Thank you very much for your detailed investigation @BlankSpruce!

You are right about the origin of the macro.

I believe 1) is the most simple and helpful way to get feedback which definitions are missing and improve the configuration of gersemi! I can confirm that handing over the specific definition makes gersemi format the file correctly :+1:

In my specific case the macro can be very hard to find automatically, because the ROS2 build system colcon allows to "install" the same package containing such macros multiple times in so called "overlay workspaces". It is probably better to leave it to the human to instruct gersemi where to find those :)

BlankSpruce commented 4 months ago

In my specific case the macro can be very hard to find automatically, because the ROS2 build system colcon allows to "install" the same package containing such macros multiple times in so called "overlay workspaces". It is probably better to leave it to the human to instruct gersemi where to find those :)

Could you elaborate a bit more? Would providing stub definition just for definitions be enough? Stub as in something like:

# ./src/cmake/stubs/rosidl_generate_interfaces.cmake
function(rosidl_generate_interfaces)
   cmake_parse_arguments(
        _ARG
        "ADD_LINTER_TESTS;SKIP_INSTALL;SKIP_GROUP_MEMBERSHIP_CHECK"
        "LIBRARY_NAME" 
        "DEPENDENCIES"
    )
    message(FATAL_ERROR "This is just stub, don't use it")
endfunction()

If stub isn't sufficient then perhaps we could work out something better if you provide some details.

remod commented 4 months ago

In ROS2, you can an arbitrary number of colcon workspaces "overlaying" each other. When you then need any resource / library / executable from a given package, the package is first searched for the in the "topmost" workspace. If it does not exist, it is searched for in the underlying workspace, etc. Therefore, path resolution is dynamic, so any "hardcoded" path like in your stub won't work for the generic case.

I hope that makes it clear :)

I'd probably just leave the task of figuring out the correct path to the .cmake definition to the user which then passes it via --definitions to gersemi. Having gersemi printing a warning in case a definition is missing is already very helpful!

BlankSpruce commented 4 months ago

I might have been misunderstood. There is no intent to have hardcoded path in the stub definition, merely something that is exposed to gersemi that provides definition of used keywords.

I hope to find some time for this feature request by the next weekend. Fortunately (1) and (1a) aren't terribly difficult to implement.

remod commented 4 months ago

Oh OK, thanks for clarifying, and thank you very much for addressing this feature request :muscle:

BlankSpruce commented 4 months ago

I've just released 0.14.0 which will produce warnings about unknown commands when you don't use either --quiet or quiet: true. I've implemented only (1) because as I realized later on doing (1a) is not exactly cheap for the user. Scanning through all the sources that are formatted/checked could be seen as an observable slowdown so for now I opted out of implementing that.

I've also updated "Let's make a deal section":

after

watch_david_fincher_movies( "Se7en" "The Game" "Fight Club" "Zodiac" "The Curious Case of Benjamin Button" )



If there are any problems with this release feel free to raise new issue.
remod commented 4 months ago

@BlankSpruce Thank you!

One (possibly stupid) question before I create a new issue:

With the new release, if I run gersemi src/anybotics/tools/time/acl_time/CMakeLists.txt, I get the formatted file via stdout and a few warnings like

Warning: unknown command 'ament_package' used at:
/colcon_ws/src/anybotics/tools/time/acl_time/CMakeLists.txt:229:5

If I run it with inexistent definitions like gersemi --definitions asdf src/anybotics/tools/time/acl_time/CMakeLists.txt, I would assume that I get the same output, however I get nothing.

Is that intended behavior?

BlankSpruce commented 4 months ago

gersemi --definitions asdf src/anybotics/tools/time/acl_time/ != gersemi --definitions asdf -- src/anybotics/tools/time/acl_time/ The first invocation formats no files with two definitions to stdout, second one formats one file with one definition to stdout.

remod commented 4 months ago

So it was indeed a stupid question :laughing:

Thank you very much @BlankSpruce, the feature is working as intended! Currently intregrating gersemi into our development toolchain and CI!! :partying_face: