Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions libs/rtemodel/include/RtePackage.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -447,6 +447,12 @@ class RtePackage : public RteRootItem
*/
void SetPackageState(PackageState packState) { m_packState = packState; }

/**
* @brief get full pack ID in YAML format
* @return full pack ID in YAML format
*/
std::string GetPackYamlID() const;

/**
* @brief get full or common pack ID
* @param withVersion flag to return full (true) or common (false) ID
Expand Down
8 changes: 7 additions & 1 deletion libs/rtemodel/src/RteGenerator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,12 +145,18 @@ string RteGenerator::GetExecutable(RteTarget* target, const std::string& hostTyp
vector<pair<string, string> > RteGenerator::GetExpandedArguments(RteTarget* target, const string& hostType) const
{
vector<pair<string, string> > args;
string packId = GetPackage()->GetPackYamlID();
string generatorId = GetGeneratorName();

RteItem* argsItem = GetArgumentsItem("exe");
if (argsItem) {
for (auto arg : argsItem->GetChildren()) {
if (arg->GetTag() != "argument" || !arg->MatchesHost(hostType))
continue;
args.push_back({arg->GetAttribute("switch"), ExpandString(arg->GetText())});
string res = arg->GetText();
RteUtils::ReplaceAll(res, "$pack_id", packId);
RteUtils::ReplaceAll(res, "$generator_id", generatorId);
Comment on lines +157 to +158
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @brondani,

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

  1. 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
  2. They are still not guaranteed to be unique globally, so we would still require the pack id to make it fully unique
  3. 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:

  1. Open-CMSIS-Pack project guarantees that Only a single vendor can provide generators for a Gvendor
  2. 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).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

  1. 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.

  1. 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.

args.push_back({arg->GetAttribute("switch"), ExpandString(res)});
}
}
return args;
Expand Down
20 changes: 20 additions & 0 deletions libs/rtemodel/src/RtePackage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -743,6 +743,26 @@ string RtePackage::ConstructID()
}


string RtePackage::GetPackYamlID() const
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
string res;

const auto& vendor = GetVendorString().empty() ? "" : GetVendorString() + "::";
const vector<pair<const char*, const string&>> elements = {
{"", vendor},
{"", GetName()},
{"@", GetVersionString()},
};

for (const auto& element : elements) {
if (!element.second.empty()) {
res += element.first + element.second;
}
}
return res;
}


string RtePackage::GetPackageID(bool withVersion) const
{
if (withVersion)
Expand Down
6 changes: 4 additions & 2 deletions tools/projmgr/schemas/common.schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -478,12 +478,13 @@
"description": "General generator info",
"type": "object",
"properties": {
"from-pack": { "$ref": "#/definitions/PackID"},
"generator": { "type": "string", "description": "Section for a specific generator" },
"path": { "type": "string", "description": "Path name for storing the files generated" },
"gpdsc": { "type": "string", "description": "File name of the *.GDPSC file that stores the information in path" },
"command": { "$ref": "#/definitions/CommandType" }
},
"required": [ "generator", "path", "gpdsc", "command" ]
"required": [ "generator", "from-pack", "path", "gpdsc", "command" ]
},
"CommandType": {
"type": "object",
Expand Down Expand Up @@ -515,9 +516,10 @@
"type": "object",
"properties": {
"id": { "type": "string" },
"from-pack": { "$ref": "#/definitions/PackID"},
"files": { "$ref": "#/definitions/FilesType" }
},
"required": [ "id" ]
"required": [ "id", "from-pack" ]
},
"ComponentID": {
"type": "string",
Expand Down
13 changes: 11 additions & 2 deletions tools/projmgr/src/ProjMgrYamlEmitter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,8 +157,16 @@ void ProjMgrYamlCbuild::SetComponentsNode(YAML::Node node, const ContextItem* co
}
SetControlsNode(componentNode, componentItem->build);
SetComponentFilesNode(componentNode[YAML_FILES], context, componentId);
SetNodeValue(componentNode[YAML_GENERATOR][YAML_ID], component.generator);
SetGeneratorFiles(componentNode[YAML_GENERATOR], context, componentId);
if (!component.generator.empty()) {
if (context->generators.find(component.generator) != context->generators.end()) {
const RteGenerator* rteGenerator = context->generators.at(component.generator);
SetNodeValue(componentNode[YAML_GENERATOR][YAML_ID], component.generator);
SetNodeValue(componentNode[YAML_GENERATOR][YAML_FROM_PACK], rteGenerator->GetPackage()->GetPackYamlID());
SetGeneratorFiles(componentNode[YAML_GENERATOR], context, componentId);
} else {
ProjMgrLogger::Warn(string("Component ") + componentId + " uses unknown generator " + component.generator);
}
}
node.push_back(componentNode);
}
}
Expand Down Expand Up @@ -204,6 +212,7 @@ void ProjMgrYamlCbuild::SetGeneratorsNode(YAML::Node node, const ContextItem* co
break;
}
}
SetNodeValue(genNode[YAML_FROM_PACK], generator->GetPackage()->GetPackYamlID());
SetNodeValue(genNode[YAML_PATH], FormatPath(workingDir, context->directories.cprj));
SetNodeValue(genNode[YAML_GPDSC], FormatPath(gpdscFile, context->directories.cprj));

Expand Down
2 changes: 2 additions & 0 deletions tools/projmgr/test/data/TestGenerator/ref/multiple-components.Debug+CM0.cbuild.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ build:
category: sourceC
generator:
id: RteTestGeneratorIdentifier
from-pack: ARM::[email protected]
Torbjorn-Svensson marked this conversation as resolved.
Show resolved Hide resolved
files:
- file: ${CMSIS_PACK_ROOT}/ARM/RteTestGenerator/0.1.0/Templates/RteTestGen.c.template
category: genSource
Expand All @@ -46,6 +47,7 @@ build:
selected-by: RteTest:CORE
generators:
- generator: RteTestGeneratorIdentifier
from-pack: ARM::[email protected]
path: ../data/TestGenerator/MultipleComponents/RteTestGeneratorIdentifier
gpdsc: ../data/TestGenerator/MultipleComponents/RteTestGeneratorIdentifier/RteTestGen_ARMCM0/RteTest.gpdsc
command:
Expand Down
2 changes: 2 additions & 0 deletions tools/projmgr/test/data/TestGenerator/ref/test-gpdsc-layer.Debug+CM0.cbuild.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,14 @@ build:
category: sourceC
generator:
id: RteTestGeneratorIdentifier
from-pack: ARM::[email protected]
- component: ARM::RteTest:[email protected]
condition: Cortex-M Device
from-pack: ARM::[email protected]
selected-by: RteTest:CORE
generators:
- generator: RteTestGeneratorIdentifier
from-pack: ARM::[email protected]
path: ../data/TestGenerator/layer/RTE/Device
gpdsc: ../data/TestGenerator/layer/RTE/Device/RteTestGen_ARMCM0/RteTest.gpdsc
command:
Expand Down
4 changes: 4 additions & 0 deletions ...s/projmgr/test/data/TestGenerator/ref/test-gpdsc-multiple-generators.Debug+CM0.cbuild.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,18 +31,21 @@ build:
category: sourceC
generator:
id: RteTestGeneratorIdentifier
from-pack: ARM::[email protected]
- component: ARM::Device:RteTest Generated Component:[email protected]
condition: RteDevice
from-pack: ARM::[email protected]
selected-by: Device:RteTest Generated Component:RteTestWithKey
generator:
id: RteTestGeneratorWithKey
from-pack: ARM::[email protected]
- component: ARM::RteTest:[email protected]
condition: Cortex-M Device
from-pack: ARM::[email protected]
selected-by: RteTest:CORE
generators:
- generator: RteTestGeneratorIdentifier
from-pack: ARM::[email protected]
path: ../data/TestGenerator/GeneratedFiles/RteTestGeneratorIdentifier
gpdsc: ../data/TestGenerator/GeneratedFiles/RteTestGeneratorIdentifier/RteTestGen_ARMCM0/RteTest.gpdsc
command:
Expand Down Expand Up @@ -71,6 +74,7 @@ build:
- ../../../../output/test-gpdsc-multiple-generators.Debug+CM0.cbuild.yml
- --foo=bar
- generator: RteTestGeneratorWithKey
from-pack: ARM::[email protected]
path: ../data/TestGenerator/GeneratedFiles/RteTestGeneratorWithKey
gpdsc: ../data/TestGenerator/GeneratedFiles/RteTestGeneratorWithKey/RteTestGen_ARMCM0/RteTest.gpdsc
command:
Expand Down
2 changes: 2 additions & 0 deletions tools/projmgr/test/data/TestGenerator/ref/test-gpdsc.Debug+CM0.cbuild.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,14 @@ build:
category: sourceC
generator:
id: RteTestGeneratorIdentifier
from-pack: ARM::[email protected]
- component: ARM::RteTest:[email protected]
condition: Cortex-M Device
from-pack: ARM::[email protected]
selected-by: RteTest:CORE
generators:
- generator: RteTestGeneratorIdentifier
from-pack: ARM::[email protected]
path: ../data/TestGenerator/RTE/Device
gpdsc: ../data/TestGenerator/RTE/Device/RteTestGen_ARMCM0/RteTest.gpdsc
command:
Expand Down
2 changes: 2 additions & 0 deletions tools/projmgr/test/data/TestSolution/ref/TestProject3_1.Debug+TypeA.cbuild.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ build:
selected-by: Device:RteTest Generated Component:RteTestGenFiles
generator:
id: RteTestGeneratorIdentifier
from-pack: ARM::[email protected]
files:
- file: ${CMSIS_PACK_ROOT}/ARM/RteTestGenerator/0.1.0/Templates/RteTest.gpdsc.template
category: genAsset
Expand Down Expand Up @@ -66,6 +67,7 @@ build:
selected-by: CORE
generators:
- generator: RteTestGeneratorIdentifier
from-pack: ARM::[email protected]
path: gendir
gpdsc: gendir/RteTestGen_ARMCM0/RteTest.gpdsc
command:
Expand Down