Make creation of RTE folder optional

[ErikMallbergSTM]

This introduces the flag --no-update-rte for the csolution convert and cbuildgen cmake commands to skip creation/update of the RTE folder and files. It is useful when you just want to resolve the project but don't want the RTE files.

In addition, the commands csolution list dependencies and csolution list generators are fixed to never create the RTE folder.

Fixes #371

Prerequisite: #436

Contributed by STMicroelectronics

Signed-off-by: Erik LUNDIN erik.lundin@st.com

[ErikMallbergSTM]

One of the integration tests failed so I pushed fixed versions of the commits.

[edriouk]

There are three problems with this PR that need to re-worked:

1. Separate RteFsUtils change in a separate PR as it is not related to the rest.
2. Add RteKernel methods to check if RTE files need to be copied. This should replace passing such flags all way down to the hierarchy.
3. Restructuring InitFileInstance() and adding WriteInstanceFiles() : all cses should be considered, not only loading cprj file

[ErikMallbergSTM]

There are three problems with this PR that need to re-worked:

Thank you for your suggestions. Regarding passing the flag down the hierarchy, I agree that it felt somewhat strange to have to do that, and I'm looking into how to implement it using RteKernel.

[ErikMallbergSTM]

2. Add RteKernel methods to check if RTE files need to be copied. This should replace passing such flags all way down to the hierarchy.

I ended up adding methods to RteModel instead of RteKernel, since the flag needs to be checked also in RteProject, which doesn't have direct access to the kernel but to the global model.

[brondani]

@ErikLundinSTM As discussed in the meeting today, let's make the inverted logic for cbuildgen cmake, in other words by default don't create/update RTE files but introduce an --update-rte flag.
Please investigate why projmgr tests and buildmgr tests are failing.
The tests must cover both behaviours (create/not create RTE) and all RTE content (config files, RTE_Components.h, pre-include files) for csolution and cbuildgen.
Feel free to contact us for any kind of support.
@edriouk Please write here your considerations so we can keep track, thanks!

[edriouk]

I do agree with the idea that updating RTE folders can be made optional.
Implementation however should be backward compatible and not break the existing client implementations.
That can be achieved by the following measures:

1. The calls to methods that copies, creates or deletes files should deletes files in the folders should remain as they are.
2. The signatures of those methods should not be changed.
3. The methods should themselves query parent RteProject if to perform the actions.
4. The RteProject can keep that flag in its attributes, e.g. as "update-rte" boolean attribute (default true). Setting that attribute can be performed with the existing methods. NOTE: cbuildgen must always explicitly set the attribute!

[github-actions]

Unit Test Results

  27 files    42 suites   1s ⏱️
106 tests 106 ✔️ 0 💤 0 ❌
318 runs  318 ✔️ 0 💤 0 ❌

Results for commit 392a336.

[ErikMallbergSTM]

@edriouk Thank you for your input. When saying that the methods that copy, create or delete files/folders should be left unchanged, do you refer to e.g. RteProject::AddCprjComponents and RteProject::InitFileInstance? They can probably be changed back to keep updating the files, based on the value of an attribute set on the project, but wouldn't that mean that we lose the ability to explicitly do all the file operations? One important idea behind this PR was to extract the RTE file operations from various places to be able to have better control of when it happens.

[ErikMallbergSTM]

Code rebased on latest main to keep up with changes there.

[edriouk]

@ErikLundinSTM, yes, I would suggest changing RteProject::AddCprjComponents and RteProject::InitFileInstance back to their previous states to keep compatibility with the existing tools first. The most important aspect here is that RteProject::InitFileInstance() gets eventually called from the following chain:
RteProject::Apply()->RteProject::Update()->AddComponentFiles()->... -> RteProject::InitFileInstance()
That chain gets executed by loading a project and every time by adding/removing components. Thus not only via RteProject::AddCprjComponents().

The obvious place to centralize file operations is RteProject::Update(). I would prefer to do that consolidation in a separate PR.

[github-actions]

Test Results

    8 files    50 suites   5m 28s ⏱️
160 tests 160 ✔️ 0 💤 0 ❌
636 runs  636 ✔️ 0 💤 0 ❌

Results for commit 6735f45.

♻️ This comment has been updated with latest results.

[ErikMallbergSTM]

I have modified the PR to not change the mentioned functions. All tests should pass now as well. Please check if there are more things you would like to have changed.

[brondani]

@ErikLundinSTM Thanks for this commit!
Please find my comments and suggestions near to the code.

[brondani]

@ErikLundinSTM
In order to conclude this PR I am proposing to fix the integration tests:
ErikMallbergSTM#1

It seems we didn't realize we need to extend the cbuild options as well. Let' tackle that afterwards.

[ErikMallbergSTM]

@ErikLundinSTM In order to conclude this PR I am proposing to fix the integration tests: ErikLundinSTM#1

I'm struggling with the integration tests right now. Seems like it works if reverting the default behavior of cbuildgen cmake (--no-update-rte vs --update-rte), so I've been trying to pinpoint where to fix this in the tests, but haven't yet succeeded to find the right places.

[brondani]

@ErikLundinSTM If you merge my proposed changes into your branch ErikMallbergSTM#1 all remaining integration tests failings should be solved. I have tested it in my fork.
If you have any question please feel free to setup a call. Thanks!

[ErikMallbergSTM]

@ErikLundinSTM If you merge my proposed changes into your branch ErikLundinSTM#1 all remaining integration tests failings should be solved. I have tested it in my fork. If you have any question please feel free to setup a call. Thanks!

Sorry, I missed that PR. Is it OK if I first remove my last, temporary commit and rebase on main? Then I'll merge your fixes to the integration tests.

[brondani]

Sorry, I missed that PR. Is it OK if I first remove my last, temporary commit and rebase on main? Then I'll merge your fixes to the integration tests.

There is no need to remove the commit - that line was actually reworked in my changes. In the end we do a "squash and merge" so all the PR changes are merged into main as one single commit. And yes you can rebase on main.

[codecov]

Codecov Report

Merging #432 (6735f45) into main (270b27a) will increase coverage by 24.37%.
The diff coverage is 66.66%.

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #432       +/-   ##
===========================================
+ Coverage   45.88%   70.26%   +24.37%     
===========================================
  Files         116       50       -66     
  Lines       20792     9701    -11091     
  Branches    11376     5963     -5413     
===========================================
- Hits         9541     6816     -2725     
+ Misses       9457     1855     -7602     
+ Partials     1794     1030      -764     

Flag	Coverage Δ
buildmgr-cov	75.20% <60.00%> (+0.10%)	⬆️
projmgr-cov	78.47% <75.00%> (+0.01%)	⬆️
svdconv-cov	?

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

Impacted Files	Coverage Δ
tools/buildmgr/cbuild/include/CbuildModel.h	90.47% <ø> (ø)
tools/buildmgr/cbuild/src/CbuildLayer.cpp	76.04% <0.00%> (ø)
tools/buildmgr/cbuild/src/CbuildModel.cpp	68.54% <50.00%> (+0.03%)	⬆️
tools/projmgr/src/ProjMgrWorker.cpp	78.22% <50.00%> (+0.01%)	⬆️
tools/buildmgr/cbuildgen/src/CbuildGen.cpp	78.09% <80.00%> (+0.42%)	⬆️
tools/projmgr/src/ProjMgr.cpp	72.80% <100.00%> (+0.16%)	⬆️
tools/svdconv/SVDModel/src/SvdEnum.cpp
tools/svdconv/SVDModel/src/SvdDimension.cpp
tools/svdconv/SVDModel/include/SvdUtils.h
tools/svdconv/SVDGenerator/src/SfdGenerator.cpp
... and 64 more

[acabarbaye]

It seems we didn't realize we need to extend the cbuild options as well. Let' tackle that afterwards.

Could cbuild be updated to have these flags available as well so that the build service can make use of this?

[brondani]

Could cbuild be updated to have these flags available as well so that the build service can make use of this?

Yes it is tracked here: Open-CMSIS-Pack/cbuild#21