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.

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

[projmgr] Generate RTE directory in project tree #1564

Conversation

Torbjorn-Svensson
Copy link
Collaborator

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

Copy link

github-actions bot commented Jun 15, 2024

Test Results

  7 files   53 suites   4m 15s ⏱️
186 tests 169 ✅ 17 💤 0 ❌
695 runs  627 ✅ 68 💤 0 ❌

Results for commit 86c7bb1.

♻️ This comment has been updated with latest results.

@brondani
Copy link
Collaborator

Thanks for reporting this. As you correctly says it was introduced in #1284 addressing #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 #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
Copy link
Collaborator Author

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
Copy link
Collaborator

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?

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.

Contributed by STMicroelectronics

Signed-off-by: Torbjörn SVENSSON <[email protected]>
@Torbjorn-Svensson Torbjorn-Svensson force-pushed the pr/failed-convert-should-create-RTE-dir-in-project-folder branch from 927d6ea to 86c7bb1 Compare June 17, 2024 10:36
@Torbjorn-Svensson
Copy link
Collaborator Author

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.

Copy link

codecov bot commented Jun 17, 2024

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
@@           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 Coverage Δ
buildmgr-cov 74.12% <ø> (ø)
packchk-cov 65.54% <ø> (ø)
packgen-cov 77.55% <ø> (ø)
projmgr-cov 80.47% <65.21%> (-0.02%) ⬇️

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

Files Coverage Δ
tools/projmgr/test/src/ProjMgrTestEnv.cpp 73.23% <100.00%> (+0.97%) ⬆️
tools/projmgr/test/src/ProjMgrUnitTests.cpp 79.59% <55.55%> (-0.12%) ⬇️

... and 7 files with indirect coverage changes

Copy link
Collaborator

@brondani brondani left a comment

Choose a reason for hiding this comment

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

@Torbjorn-Svensson Thanks for the contribution.
LGTM

@brondani brondani merged commit 0f1bfe8 into Open-CMSIS-Pack:main Jun 17, 2024
100 of 101 checks passed
@Torbjorn-Svensson Torbjorn-Svensson deleted the pr/failed-convert-should-create-RTE-dir-in-project-folder branch June 17, 2024 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants