search

Open-CMSIS-Pack / devtools

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

[projmgr] Generate RTE directory in project tree #1564

Closed Torbjorn-Svensson closed 3 weeks ago

Torbjorn-Svensson commented 3 weeks ago

When running csolution convert -s /absolute/path/to/foo.csolution.yml, the "RTE" directory should be placed within that structure and not where the csolution process happens to be executed from.

This is a regression in csolution 2.3.0 compared to 2.2.1 and was introduced by this change. In the commit, the RTE directory is now generated without initializing the project, so the path to the project will be the empty string rather than the actual location on disk and as a result, it's created in the working directory rather than in the project tree.

I've added a test case to demonstrate the problem, but I don't know how to fix it. @jkrech | @edriouk: Can please you take a look?

Contributed by STMicroelectronics

github-actions[bot] commented 3 weeks ago

Test Results

  7 files   53 suites   4m 15s :stopwatch: 186 tests 169 :white_check_mark: 17 :zzz: 0 :x: 695 runs  627 :white_check_mark: 68 :zzz: 0 :x:

Results for commit 86c7bb17.

:recycle: This comment has been updated with latest results.

brondani commented 3 weeks ago

Thanks for reporting this. As you correctly says it was introduced in https://github.com/Open-CMSIS-Pack/devtools/pull/1284 addressing https://github.com/Open-CMSIS-Pack/devtools/issues/1266. Please note it was not a regression: prior to that change the convert processing was fully stopped when the first error was found, so nothing was created, while the required change purposely continue the processing also in case of errors. Importantly the return code is different from 0 and the cbuild-idx file brings the errors: true node.

As a matter of fact the generation of RTE_Components.h was suppressed in https://github.com/Open-CMSIS-Pack/devtools/pull/1527 when no components are selected, so your test case seems not to be valid anymore. If I am missing something please amend the PR.

Torbjorn-Svensson commented 3 weeks ago

While it's true that nothing was produced earlier, it's still a regression since now, there is a random RTE directory created in the directory you launched the csolution binary from. Lets say that you are standing in /path/to/some/read-only/installation/directory/ when you start csolution. This would cause other errors to pop-up. Even worse is that you might stand in a different project and just needs to run csolution convert with an absolute path to the other project and now the project in cwd is corrupted.

I would say that there are 2 possible solutions:

  1. Only create the RTE directory if project was successfully parsed. (more or less a revert...)
  2. Always initialize where the project is located and generate it there.

The current behavior is unpredictable and may introduce other problems for the end-user.

This issue is not about that the identifier that something went wrong has changed, but about that a stray RTE directory can be placed anywhere, not just in the project where it's supposedly to be generated (due to the uninitialized internal state). From what I can see, this statement needs to always be executed prior to creating the RTE directory: https://github.com/Open-CMSIS-Pack/devtools/blob/main/tools/projmgr/src/ProjMgrWorker.cpp#L556

brondani commented 3 weeks ago

Only create the RTE directory if project was successfully parsed

This seems to be already the case since https://github.com/Open-CMSIS-Pack/devtools/pull/1527, can you please fix the unittests building issues reported by CI and update the test case?

Torbjorn-Svensson commented 3 weeks ago

Only create the RTE directory if project was successfully parsed

This seems to be already the case since #1527, can you please fix the unittests building issues reported by CI and update the test case?

Updated the test case and it appears to work now. Not sure how I could have missed this, as I know that I intended to test with main, but maybe I forgot to build after I found the commit that caused the problem... Anyway, I think the test case is valuable regardless as it's the only test cases that tries to run csolution in a different directory than the .csolution.yml file is located in.

codecov[bot] commented 3 weeks ago

Codecov Report

Attention: Patch coverage is 65.21739% with 8 lines in your changes missing coverage. Please review.

Project coverage is 63.47%. Comparing base (17b2ab1) to head (86c7bb1). Report is 2 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1564 +/- ## ======================================= Coverage 63.46% 63.47% ======================================= Files 162 162 Lines 31080 31103 +23 Branches 19036 19052 +16 ======================================= + Hits 19726 19743 +17 - Misses 7517 7524 +7 + Partials 3837 3836 -1 ``` | [Flag](https://app.codecov.io/gh/Open-CMSIS-Pack/devtools/pull/1564/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Open-CMSIS-Pack) | Coverage Δ | | |---|---|---| | [buildmgr-cov](https://app.codecov.io/gh/Open-CMSIS-Pack/devtools/pull/1564/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Open-CMSIS-Pack) | `74.12% <ø> (ø)` | | | [packchk-cov](https://app.codecov.io/gh/Open-CMSIS-Pack/devtools/pull/1564/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Open-CMSIS-Pack) | `65.54% <ø> (ø)` | | | [packgen-cov](https://app.codecov.io/gh/Open-CMSIS-Pack/devtools/pull/1564/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Open-CMSIS-Pack) | `77.55% <ø> (ø)` | | | [projmgr-cov](https://app.codecov.io/gh/Open-CMSIS-Pack/devtools/pull/1564/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Open-CMSIS-Pack) | `80.47% <65.21%> (-0.02%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Open-CMSIS-Pack#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files](https://app.codecov.io/gh/Open-CMSIS-Pack/devtools/pull/1564?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Open-CMSIS-Pack) | Coverage Δ | | |---|---|---| | [tools/projmgr/test/src/ProjMgrTestEnv.cpp](https://app.codecov.io/gh/Open-CMSIS-Pack/devtools/pull/1564?src=pr&el=tree&filepath=tools%2Fprojmgr%2Ftest%2Fsrc%2FProjMgrTestEnv.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Open-CMSIS-Pack#diff-dG9vbHMvcHJvam1nci90ZXN0L3NyYy9Qcm9qTWdyVGVzdEVudi5jcHA=) | `73.23% <100.00%> (+0.97%)` | :arrow_up: | | [tools/projmgr/test/src/ProjMgrUnitTests.cpp](https://app.codecov.io/gh/Open-CMSIS-Pack/devtools/pull/1564?src=pr&el=tree&filepath=tools%2Fprojmgr%2Ftest%2Fsrc%2FProjMgrUnitTests.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Open-CMSIS-Pack#diff-dG9vbHMvcHJvam1nci90ZXN0L3NyYy9Qcm9qTWdyVW5pdFRlc3RzLmNwcA==) | `79.59% <55.55%> (-0.12%)` | :arrow_down: | ... and [7 files with indirect coverage changes](https://app.codecov.io/gh/Open-CMSIS-Pack/devtools/pull/1564/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Open-CMSIS-Pack)