Define $generator_id and $pack_id variables in generator arguments #1007

Torbjorn-Svensson · 2023-06-22T09:42:22Z

$generator_id resolves to the current generator id
$pack_id resolves to the current pack id in YAML format

Also, add from-pack property on the generator nodes in the cbuild.yml to identify what pack the generator is defined in. This resolves potential conflicts when using generators from multiple packs that has the same name.

Contributed by STMicroelectronics

$generator_id resolves to the current generator id $pack_id resolves to the current pack id in YAML format Also, add from-pack property on the generator nodes in the cbuild.yml to identify what pack the generator is defined in. This resolves potential conflicts when using generators from multiple packs that has the same name. Contributed by STMicroelectronics Signed-off-by: Torbjörn SVENSSON <[email protected]>

codecov · 2023-06-22T09:49:25Z

Codecov Report

Merging #1007 (a46782d) into main (22d2212) will increase coverage by 0.00%.
The diff coverage is 75.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1007   +/-   ##
=======================================
  Coverage   54.31%   54.32%           
=======================================
  Files         116      116           
  Lines       22538    22544    +6     
  Branches    12498    12504    +6     
=======================================
+ Hits        12242    12246    +4     
- Misses       8148     8149    +1     
- Partials     2148     2149    +1

Flag	Coverage Δ
buildmgr-cov	`77.51% <ø> (ø)`
packchk-cov	`60.60% <ø> (ø)`
projmgr-cov	`82.18% <75.00%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
tools/projmgr/src/ProjMgrYamlEmitter.cpp	`92.15% <75.00%> (-0.40%)`	⬇️

github-actions · 2023-06-22T09:51:02Z

Test Results

    7 files   53 suites 4m 48s ⏱️
184 tests 167 ✔️ 17 💤 0 ❌
685 runs 617 ✔️ 68 💤 0 ❌

Results for commit a46782d.

♻️ This comment has been updated with latest results.

jkrech · 2023-06-22T10:26:25Z

In previous discussions it was our goal to make all information required by the generator via the cbuild.yml file of the current context. Why do we need these variables to be passed to the generator via the command line? What has changed and what current problem does it solve?

Torbjorn-Svensson · 2023-06-22T10:33:53Z

In previous discussions it was our goal to make all information required by the generator via the cbuild.yml file of the current context. Why do we need these variables to be passed to the generator via the command line? What has changed and what current problem does it solve?

In order for the generator application to collect the required info from the cbuild.yml, it needs to be able to properly identify what node to use. To avoid the ambiguity, the $pack_id symbol is used to identify in what pack the generator is defined (more than one pack could have a generator with the same name...).
The $generator_id is just for convenience to not have to write the same string over and over in the .pdsc file.

@slhultgren, please fill in if I missed something.

brondani · 2023-06-26T10:03:53Z

libs/rtemodel/src/RtePackage.cpp

@@ -743,6 +743,26 @@ string RtePackage::ConstructID()
 }


+string RtePackage::GetPackYamlID() const


Couldn't string ProjMgrUtils::GetPackageID(const RteItem* pack) be used instead of introducing this function?

The class ProjMgrUtils is part of the projmgr tool, so either the class or function needs to be migrated to some other location in the code base or it needs to be duplicated like I've done.
Would you rather have me move ProjMgrUtils::GetPackageID to the RtePackage class?

Not sure if we want to introduce the YAML format awareness into the RtePackage or if it's better to keep it limited to the projmgr tool.
As previously commented the variables expansion should be done via callbacks which can be tool's specific.
@edriouk can you please advise?

We are already considering to unify component, API, and pack IDs across tools and libraries. RTE model will use then format used in .cproject/.csolution. This happens not for 2.0 release, but soon. therefore I agree with @brondani not to introduce extra methods in RTE model.

Do you have a way to fetch the generator of interest from the callback class?
As far as I can tell, only global context is available and no way to get the current generator.

brondani · 2023-06-26T10:19:03Z

libs/rtemodel/src/RteGenerator.cpp

+      RteUtils::ReplaceAll(res, "$pack_id", packId);
+      RteUtils::ReplaceAll(res, "$generator_id", generatorId);


It seems $pack_id and $generator_id are deviating from the standard way of declaring/expanding/using variables such as $P #P $S $D $B $G:
https://open-cmsis-pack.github.io/Open-CMSIS-Pack-Spec/main/html/pdsc_generators_pg.html#element_generators
Concerning implementation look at RteCallback::ExpandString.

@slhultgren We have discussed about the use of [Gvendor][::Gtool][@Gversion] as generator id. As long as the vendor guarantees the Gtool uniqueness, wouldn't it be sufficient?
#434 (comment)

It was intention to use longer names than a single letter as it's hard to give an idea of what the variable means with only one letter.

The vendor can never guarantee that the generator id is unique among all combinations of packs, so by just specifying what pack the generator comes from, it will allow it to be uniquely identifiable in the cbuild.yml file.

Hi @brondani,

I'm not sure to catch the point about Gvendor/Gtool/Gversion, since my problem currently with these are:

I don't understand what/how they are used
1.1. Gtool is defined as "Plain text name", this sounds almost like a human-readable description, not something to rely on functionally
1.2. There is no implementation yet supporting this AFAIK

They are still not guaranteed to be unique globally, so we would still require the pack id to make it fully unique

If Gvendor must follow the pack vendor, not sure why it is even possible to define it for the generator

Technically I guess Gvendor/Gtool/Gversion could work, as long as:

Open-CMSIS-Pack project guarantees that Only a single vendor can provide generators for a Gvendor

The Vendor guarantees that it does not release multiple conflicting generators using the same Gtool

but honestly I don't think any of these two points are feasible in practice (not worth the effort; The alternative of just allowing to specify the pack id is a lot simpler to maintain and double check on an organizational level. Each pack is more self-contained).

Currently my view on the Gvendor/Gtool/Gversion is that they provide some abstraction for calling generators (potentially from other packs??) but I don't think this level of abstraction is necessary or maybe not even useful?
Most likely when defining a component using a generator, we will know/expect some kind of behavior of the generator that we call, so most likely we just want to be able to call a generator from (at most) another pack (that we already know).

@slhultgren

1.1. Gtool is defined as "Plain text name", this sounds almost like a human-readable description, not something to rely on functionally

Not really different from other identifiers such as the pack name itself.

1.2. There is no implementation yet supporting this AFAIK

Right, if we want to benefit from the spec we should start implementing it. I thought we agreed here this was the best approach to follow?

They are still not guaranteed to be unique globally, so we would still require the pack id to make it fully unique

Gvendor should be unique. Each vendor would be responsible for their own Gtool identifiers.

If Gvendor must follow the pack vendor, not sure why it is even possible to define it for the generator

I guess for the same reason it’s possible to specify Cvendor for components: if omitted it can be inherited from pack vendor, but it does not have to.

If you now strongly believe the from-pack is a better approach, I would recommend to update the specification.

tools/projmgr/test/data/TestGenerator/ref/multiple-components.Debug+CM0.cbuild.yml

slhultgren · 2023-08-25T15:14:00Z

@jkrech @brondani
Hi, just looked at the recording of the meeting from 2023-08-22 at ~47 minutes, I think this PR is actually pretty close and aligned with the discussed proposal for the generator-pack and generator-id additions to the cbuild.yml.

If I understood correctly the presentation, the proposed generator-pack, and generator-id are actually a kind of current-generator-pack and current-generator-id since each invocation of a generator would get its' own unique cbuild.yml created for it.

The "minor" adjustments needed on this PR to align with that proposal I think would be to

Remove the $pack_id and $generator_id variables from the .pdsc generator argument specification, no need anymore since effectively replaced by the "cbuild.yml - generator-pack/id" (and also close Define $pack_id and $generator_id for generator arguments Open-CMSIS-Pack-Spec#238)
The generator-pack and generator-id properties should be added to the cbuild.yml when running a generator.
2.1. Proposal is that since the cbuild.yml could be created for contexts without any generators as well, perhaps it would make sense to group these two properties in an optional object, eg.

build:
  ...
  current-generator:
    from-pack: Keil::[email protected]
    id: STM32CubeMX

Where the current-generator object is optional, but the from-pack and id properties on the object are both mandatory if the object is included.
(Naming to be discussed as always 😄)

Implications:

The cbuild.yml when running a generator is always generator+context specific and always temporary (similar to the general idea of the other PR with cbuild.resolved.yml Create *.cbuild-gen.yml with absolute paths #975)

jkrech · 2023-08-28T08:13:52Z

What if we take this one step further by creating a cbuild.yml file dedicated to an invoked generator, located side by side with the cbuild.yml for the context named e.g.:

<context>.cbuild-<generator-id>.yml

which contains both the generatorID and the packID of the generator and provides absolute paths for all file references to ease the use by the generator. $G is replaced by a reference to the above file.

These generator specific *.cbuild-generatorID.yml files are considered temporary as they are most likely out-of-date after the generator has completed and csolution re-reads the context information.

Any thoughts?

jkrech · 2023-09-18T12:24:39Z

We agreed during the technical project meeting to include the generatorID and packID of the generator into the <context>.cbuild-gen.yml fileinstead of extending the list of $variables to be placed at the command line. Instead the generator invocation will use$G` to specify the location of the cbuild-gen.yml file for the given context.

adding e.g.:

build:
  generated-by: csolution version 2.1.0
  generator-pack: Keil::[email protected]
  generator-id: STM32CubeMX

Torbjorn-Svensson · 2023-09-20T14:28:26Z

Superseded by #1129

To facilitate implementation of generators, create `*.cbuild-gen.yml` with: - any file and directory paths inside expanded to `absolute paths` in cbuild-gen.yml. - new node, `current-generator` that can be used to identify what generator node in the cbuild-gen.yml that the current invocation is for. Both `*.cbuild.yml` and `*.cbuild-gen.yml` are created inside the `intdir`, since they are both temporary files used by generators and builders. The `$G` parameter for generators now expands to the `*.cbuild-gen.yml` file. In both *.cbuild.yml and *.cbuild-gen.yml, all generator nodes now also includes the `from-pack` node that defines in what pack that the generator was defined. For more background, see [2023-08-22 Open-CMSIS-Pack meeting](https://linaro.atlassian.net/wiki/spaces/CMSIS/pages/28990341176/Open-CMSIS-Pack+Technical+Meeting+2023-08-22) and #1007. Example `cbuild-gen.yml`: ```yml build: generated-by: csolution version 2.2.0 current-generator: id: RteTestGeneratorIdentifier from-pack: ARM::[email protected] ``` Example `cbuild.yml` with new `from-pack` node on `generator-nodes`: ```yml build: components: - component: ARM::Device:RteTest Generated Component:[email protected] generator: id: RteTestGeneratorIdentifier from-pack: ARM::[email protected] generators: - generator: RteTestGeneratorIdentifier from-pack: ARM::[email protected] ``` Contributed by STMicroelectronics --------- Signed-off-by: Torbjörn SVENSSON <[email protected]> Signed-off-by: Erik MÅLLBERG <[email protected]> Co-authored-by: Torbjörn SVENSSON <[email protected]> Co-authored-by: Erik MÅLLBERG <[email protected]> Co-authored-by: Samuel HULTGREN <[email protected]>

…MSIS-Pack#1007)

Torbjorn-Svensson mentioned this pull request Jun 22, 2023

Define $pack_id and $generator_id for generator arguments Open-CMSIS-Pack/Open-CMSIS-Pack-Spec#238

Closed

jkrech requested review from brondani and JonatanAntoni June 22, 2023 10:26

brondani reviewed Jun 26, 2023

View reviewed changes

tools/projmgr/test/data/TestGenerator/ref/multiple-components.Debug+CM0.cbuild.yml Show resolved Hide resolved

jkrech closed this Sep 18, 2023

Torbjorn-Svensson mentioned this pull request Sep 20, 2023

Implement *.cbuild-gen.yml #1129

Merged

Torbjorn-Svensson deleted the pr/generator-pack-from-pack branch September 20, 2023 14:28

grasci-arm pushed a commit to ARM-software/devtools that referenced this pull request Aug 29, 2024

[projmgr] Add PLM status into config file nodes in cbuild.yml (Open-C…

cc36afb

…MSIS-Pack#1007)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Define $generator_id and $pack_id variables in generator arguments #1007

Define $generator_id and $pack_id variables in generator arguments #1007

Torbjorn-Svensson commented Jun 22, 2023

codecov bot commented Jun 22, 2023

github-actions bot commented Jun 22, 2023 •

edited

Loading

jkrech commented Jun 22, 2023

Torbjorn-Svensson commented Jun 22, 2023 •

edited

Loading

brondani Jun 26, 2023

Torbjorn-Svensson Jun 26, 2023

brondani Jun 27, 2023

edriouk Jun 27, 2023

Torbjorn-Svensson Jun 27, 2023

brondani Jun 26, 2023

Torbjorn-Svensson Jun 26, 2023

slhultgren Aug 18, 2023

brondani Aug 18, 2023

slhultgren commented Aug 25, 2023 •

edited

Loading

jkrech commented Aug 28, 2023

jkrech commented Sep 18, 2023

Torbjorn-Svensson commented Sep 20, 2023

		@@ -743,6 +743,26 @@ string RtePackage::ConstructID()
		}


		string RtePackage::GetPackYamlID() const

		RteUtils::ReplaceAll(res, "$pack_id", packId);
		RteUtils::ReplaceAll(res, "$generator_id", generatorId);

Define $generator_id and $pack_id variables in generator arguments #1007

Define $generator_id and $pack_id variables in generator arguments #1007

Conversation

Torbjorn-Svensson commented Jun 22, 2023

codecov bot commented Jun 22, 2023

Codecov Report

github-actions bot commented Jun 22, 2023 • edited Loading

Test Results

jkrech commented Jun 22, 2023

Torbjorn-Svensson commented Jun 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slhultgren commented Aug 25, 2023 • edited Loading

jkrech commented Aug 28, 2023

jkrech commented Sep 18, 2023

Torbjorn-Svensson commented Sep 20, 2023

github-actions bot commented Jun 22, 2023 •

edited

Loading

Torbjorn-Svensson commented Jun 22, 2023 •

edited

Loading

slhultgren commented Aug 25, 2023 •

edited

Loading