Open-CMSIS-Pack / devtools

Open-CMSIS-Pack development tools - C++
Apache License 2.0
72 stars 54 forks source link

Variable description in YAML files is not backward/forward compatible #1665

Open edriouk opened 1 month ago

edriouk commented 1 month ago

Currently variables node in YAML files does provide a variable name implicitly. For instance the variable name in the following example is Board-Layer:

     variables:
         - Board-Layer: $SolutionDir()$/../../packs/EVK-MIMXRT1060_BSP/boards/evkmimxrt1060/layer/Board.clayer.yml
           description: EVK-MIMXRT1060 Development Board
           file: Board.clayer.yml
           copy-to: Board/EVK-MIMXRT1060

There are only three ways to parse such YAML:

  1. Parser knows the complete list of variable names adding a variable needs to update all parsers, not only csolution one! => not backward/forward compatible
  2. Variable name is taken from the very first element some YAML parsers may sort map elements in an arbitrary order (e.g. alphabetically) => this might not work properly
  3. The name-value variable pair is discovered as an element with an unknown key introducing new properties to a variable will brake the discovery mechanism

Suggestion: use explicitly name and value keys:

     variables:
         - name: Board-Layer: 
           value: $SolutionDir()$/../../packs/EVK-MIMXRT1060_BSP/boards/evkmimxrt1060/layer/Board.clayer.yml
           description: EVK-MIMXRT1060 Development Board
           file: Board.clayer.yml
           copy-to: Board/EVK-MIMXRT1060

This approach:

brondani commented 1 month ago

Looking at the schema the layer variable key in cbuild-idx.yml must have the pattern ^.*-Layer$ https://github.com/Open-CMSIS-Pack/devtools/blob/schemas/projmgr/2.5.0/tools/projmgr/schemas/common.schema.json#L995 For this reason it should be reasonably easy to parse and distinguish this key from other current or future siblings.

However I tend to agree the proposed change makes it easier to parse and we could go further and handle any variable, waiving the requirement of a -Layer suffix. But such change would potentially break compatibility, old and new nodes would be needed until the next major version increment.

ReinhardKeil commented 1 month ago

The problem exists only for the *.cbuild.idx. Today only two different layers are supported:

A potential way to solve generic (for more layers) in the long run is:

     layers:
         - variable: Board-Layer     # alternatively 'type: Board'
           path: $SolutionDir()$/../../packs/EVK-MIMXRT1060_BSP/boards/evkmimxrt1060/layer/Board.clayer.yml
           description: EVK-MIMXRT1060 Development Board
           file: Board.clayer.yml
           copy-to: Board/EVK-MIMXRT1060
ReinhardKeil commented 2 weeks ago

We need to conclude here. Is a change really required given the fact that key values have the format ^.*-Layer$