Open-CMSIS-Pack / devtools

Open-CMSIS-Pack development tools - C++
Apache License 2.0
69 stars 50 forks source link

Incorrect $input$ and $output$ list for executing external commands #1545

Closed RobertRostohar closed 22 hours ago

RobertRostohar commented 1 month ago

Describe the bug $input$ and $output$ used for executing external commands should contain a comma (,) separated list of files according to the specification. However the lists use a semicolon (;) as separator. Also files which are specified explicitly should not be expanded to an absolute path.

To Reproduce Create a simple solution/project with main.c and empty files data/1.txt, data/2.txt and add the following executes node:

  executes:
    - execute: Test execution
      run: echo $input$ -o $output$
      input:
       - main.c
       - data/1.txt
       - data/2.txt
      output:
        - test.out
        - test.lib

Run cbuild *.csolution.yml --cbuild2cmake

The output shows the following (assuming the solution/project is in directory D:/Temp/test):

D:/Temp/test/main.c;D:/Temp/test/data/1.txt;D:/Temp/test/data/2.txt -o D:/Temp/test/test.out;D:/Temp/test/test.lib

The expected output:

main.c,data/1.txt,data/2.txt -o test.out,test.lib

Expected behavior $input$ and $output$ should use a comma ( ,) as separator. Explicitly specified files should not be expanded to an absolute path. This would prohibit it to use with programs that process also the filename (ex: FCARM file converter).

Environment:

jkrech commented 1 month ago

The request for relative paths above imply that there is a notion of "current directory" when executing the command. This seems to depend on the directory of the YAML input file.

ReinhardKeil commented 1 month ago

@RobertRostohar what is the problem with absolute paths to input/output files?

RobertRostohar commented 1 month ago

One issue could be a very long command string when using a large number of files. The other potential issue is that the filename itself might be used by an external command. FCARM for example uses the input filename also to create a file system that is used to access the resource (ex: web/index.html). Luckily FCARM provides a ROOT directive which could be used and set to the solution/project directory.

brondani commented 1 month ago

Semicolon was preferred over comma because CMake uses it natively as lists separator: https://cmake.org/cmake/help/latest/manual/cmake-language.7.html#lists Nevertheless the transformation from semicolon to comma should be trivial if required.

To unambiguously reference files in CMake targets I would highly recommend to fully specify them in the generated CMakeLists. For this reason I would rather propose to change the access sequences behaviour (or add new ones) to strip the paths if needed.

However I'm not sure whether the FCARM parameter formatting should mandate the default behaviour of executes. Is this usual, simple and generic enough? As already mentioned in a previous comment the stripped filenames would also require to set the working directory, which was not yet needed in the current approach.

For example concerning the use of FCARM in uVision, it seems the parameters are written in a file which is then invoked in the command line: FCARM.exe @.\Auto_FcArm_Cmd.inp, that should also avoid shell/OS command line length limitation regardless the number of files.

ReinhardKeil commented 1 month ago

What is today the length limit for a command line? Can we make the delimiter configurable?

brondani commented 1 month ago

What is today the length limit for a command line?

In the Windows command prompt (cmd.exe) it seems to be 8191 characters: https://learn.microsoft.com/en-us/troubleshoot/windows-client/shell-experience/command-line-string-limitation

Can we make the delimiter configurable?

If it's meaningful, sure.

brondani commented 1 month ago

CMake scripting can be used to extend the capabilities of an execute node, for instance for formatting parameters and setting the working directory as required by FCARM in this discussion.

Consider the HTTP_Server example from STM32F4xx_DFP that uses FCARM.

An execute node for handling its FCARM task could be written in this form:

    - execute: Run FCARM
      run: ${CMAKE_COMMAND} -P $input(0)$ $input$ $output$
      input:
        - script/fcarm.cmake
        - Web
        - Web/index.htm
        - Web/pg_header.inc
        - Web/pg_footer.inc
        - Web/ad.cgi
        - Web/ad.cgx
        - Web/buttons.cgi
        - Web/buttons.cgx
        - Web/language.cgi
        - Web/lcd.cgi
        - Web/leds.cgi
        - Web/network.cgi
        - Web/system.cgi
        - Web/tcp.cgi
        - Web/xml_http.js
        - Web/home.png
        - Web/keil.gif
        - Web/logo.gif
        - Web/llblue.jpg
        - Web/pabb.gif
      output:
        - project/Web.c

And the fcarm.cmake script could have the following content:

LIST(GET CMAKE_ARGV3 1 INPUT_DIRECTORY)
LIST(REMOVE_AT CMAKE_ARGV3 0 1)
foreach(ITEM ${CMAKE_ARGV3})
  cmake_path(GET ITEM FILENAME FILE)
  LIST(APPEND FILES ${FILE})
endforeach()
string(REPLACE ";" "," FILES "${FILES}")

cmake_path(RELATIVE_PATH CMAKE_ARGV4 BASE_DIRECTORY ${INPUT_DIRECTORY} OUTPUT_VARIABLE OUTPUT)
cmake_path(GET INPUT_DIRECTORY FILENAME INPUT_DIRECTORY_NAME)
cmake_path(GET INPUT_DIRECTORY PARENT_PATH WORKING_DIRECTORY)

execute_process(COMMAND
  fcarm ${FILES} TO ${OUTPUT} RTE NOPRINT ROOT(${INPUT_DIRECTORY_NAME})
  WORKING_DIRECTORY ${WORKING_DIRECTORY}
)

This example generates an Web.c file with identical content (except for timestamp) as when called by uVision.

RobertRostohar commented 4 weeks ago

In case for FCARM, it would be simpler for the user to just populate the execute node with the input/output files and calling FCARM directly. Having an additional fcarm.cmake script increases the complexity.

ReinhardKeil commented 3 weeks ago

I believe we should focus just on the essentials. In this case FCARM generates an output file that is part of the project. The input files used for this project could be defined in a CMake project.

Assuming that CMake checks for the dependencies (to the input files) it would call the FCARM step.

brondani commented 3 weeks ago

In CMake Script Mode no configure or generate step is performed. It means adding targets and processing dependencies is not supported. For certain use cases, for example when handling a huge number of input files, it could be useful to use a standalone CMake project described via CMakeLists.txt instead of a CMake script. In such case the input dependencies can be processed in the standalone project and for instance features like file globbing can be used.

For example the execute node could be like this:

    - execute: Run FCARM
      run: ${CMAKE_COMMAND} -G Ninja -S ${INPUT_0} -B ${CMAKE_CURRENT_BINARY_DIR}/fcarm-cmake -DINPUT_DIRECTORY=${INPUT_1} -DOUTPUT=${OUTPUT} && cmake --build ${CMAKE_CURRENT_BINARY_DIR}/fcarm-cmake -- --quiet
      always:
      input:
        - fcarm-cmake
        - Web
      output:
        - project/Web.c

And the fcarm-cmake/CMakeLists.txt:

cmake_minimum_required(VERSION 3.22)
include(ExternalProject)

project("FCARM" NONE)

file(GLOB INPUT ${INPUT_DIRECTORY}/*)

foreach(ITEM ${INPUT})
  cmake_path(GET ITEM FILENAME FILE)
  list(APPEND FILES ${FILE})
endforeach()
string(REPLACE ";" "," FILES "${FILES}")

cmake_path(RELATIVE_PATH OUTPUT BASE_DIRECTORY ${INPUT_DIRECTORY} OUTPUT_VARIABLE RELATIVE_OUTPUT)
cmake_path(GET INPUT_DIRECTORY FILENAME INPUT_DIRECTORY_NAME)
cmake_path(GET INPUT_DIRECTORY PARENT_PATH WORKING_DIRECTORY)

add_custom_target(FCARM ALL DEPENDS ${OUTPUT})
add_custom_command(OUTPUT ${OUTPUT} DEPENDS ${INPUT}
  COMMAND fcarm ${FILES} TO ${RELATIVE_OUTPUT} RTE NOPRINT ROOT(${INPUT_DIRECTORY_NAME})
  WORKING_DIRECTORY ${WORKING_DIRECTORY}
)

The examples can be found in the attached http-server.zip

ReinhardKeil commented 3 weeks ago

Yes, this looks already quite good. When I understand it correctly, it includes all files that are in the directory "Web". Potential issues:

jkrech commented 2 weeks ago

Using the standard configure + build steps of cmake with CMakeLists.txt we can check for changes in the Web folder, generate a command input file and then invoke fcarm if required. There is some complexity in the run: node for people unfamiliar with cmake invokation, that in fact needs to be documented We could ship such CMakeLists.txt files as template files as part of components.

ReinhardKeil commented 2 weeks ago

This is IMHO the right solution.

We should investigate if UNIX-style file patterns could be also supported, for example:

 input:
    - ./web/**       # include all files and sub-directories

However this is not a priority. Maybe it should be the default behavior for FCARM to include all sub-directories.

jkrech commented 22 hours ago

Closing issue here and raised https://github.com/ARM-software/MDK-Middleware/issues/33 in MDK-Middleware.