Skip to content

Issue 480, improve performance of update-index#496

Merged
bgn42 merged 16 commits intomainfrom
issue_480
Apr 4, 2025
Merged

Issue 480, improve performance of update-index#496
bgn42 merged 16 commits intomainfrom
issue_480

Conversation

@bgn42
Copy link
Collaborator

@bgn42 bgn42 commented Mar 30, 2025

Fixes

#480

Changes

  • maintain version info in index.pidx with current downloaded pdsc files
  • added list of pdsc's with download errors to return to old version in index.pidx

Checklist

  • 🤖 This change is covered by unit tests (if applicable).
  • 🤹 Manual testing has been performed (if necessary).
  • 🛡️ Security impacts have been considered (if relevant).
  • 📖 Documentation updates are complete (if required).
  • 🧠 Third-party dependencies and TPIP updated (if required).

bgn42 added 2 commits March 20, 2025 17:02
added list of pdsc's with download errors to return to old version inindex.pidx
@bgn42 bgn42 requested a review from jkrech March 30, 2025 12:17
@jkrech jkrech requested a review from soumeh01 March 31, 2025 06:14
@jkrech
Copy link
Member

jkrech commented Mar 31, 2025

I am testing the version that was built from this PR:

It is much faster, however:
a) if I run a cpackget add ARM::CMSIS-FreeRTOS multiple times, cpackget keeps downloading the index.pidx over and over again. What are we checking before we update the index.pidx? update.cfg` reads:

Date=31-3-2025
Auto=false

b) I: Infineon::CAT1B_DFP was updated from latest version "1.0.0" to "1.1.0"
I: Downloading Infineon.CAT1B_DFP.pdsc...
I: 100% |██████████████████████████████████████████████████████████████████████████████| (123/123 kB, 248 kB/s)

The output does not make much sense in "past tense" the updated pdsc file with the new version is downloaded after the message.

I: Infineon::CAT1B_DFP has a new public version "1.1.0" with previous version "1.0.0"

Does that work better?

c) It is quite annoying that the version of the pack already installed is not displayed:

I: Adding pack "ARM::CMSIS-FreeRTOS"
E: Pack "ARM::CMSIS-FreeRTOS" is already installed here: "C:\Users\jkrech\AppData\Local\Arm\Packs\ARM\CMSIS-FreeRTOS", use the --force-reinstall (-F) flag to force installation

I would like to see the @version as part of the packID and for consistency the folder should include the version folder

E: Pack "ARM::CMSIS-FreeRTOS@1.0.0" is already installed here: "C:\Users\jkrech\AppData\Local\Arm\Packs\ARM\CMSIS-FreeRTOS/1.0.0", 

@bgn42
Copy link
Collaborator Author

bgn42 commented Mar 31, 2025

The file index.pidx is always read as part of the "online"-check. But now it is read twice because the Viper library cannot parse .cfg files anymore and the time check fails for that reason. I'll have a deeper look at this now. Maybe I should not use Viper for that. I already removed Viper for writing that file because of problems in that library.

@bgn42
Copy link
Collaborator Author

bgn42 commented Mar 31, 2025

@jkrech:
For case c I need to find that verseion somehow. currently it is not known.

c) It is quite annoying that the version of the pack already installed is not displayed:

I: Adding pack "ARM::CMSIS-FreeRTOS"
E: Pack "ARM::CMSIS-FreeRTOS" is already installed here: "C:\Users\jkrech\AppData\Local\Arm\Packs\ARM\CMSIS-FreeRTOS", use the --force-reinstall (-F) flag to force installation

I would like to see the @Version as part of the packID and for consistency the folder should include the version folder

E: Pack "ARM::CMSIS-FreeRTOS@1.0.0" is already installed here: "C:\Users\jkrech\AppData\Local\Arm\Packs\ARM\CMSIS-FreeRTOS/1.0.0",

@jkrech
Copy link
Member

jkrech commented Apr 1, 2025

Great: the version is displayed now:

E: Pack "NXP::LPC834_DFP@25.03.00" is already installed here: "C:\Users\jkrech\AppData\Local\Arm\Packs\NXP\LPC834_DFP\25.03.00", use the --force-reinstall (-F) flag to force installation

I think we should use a smaller file for the connection check?

https://www.keil.com/pack/Keil.vidx (12KB)

@qlty-cloud-legacy
Copy link

Code Climate has analyzed commit 59b8332 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 58.2% (50% is the threshold).

This pull request will bring the total coverage in the repository to 56.4% (-0.6% change).

View more on Code Climate.

Copy link
Member

@jkrech jkrech left a comment

Choose a reason for hiding this comment

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

LGTM

@bgn42 bgn42 merged commit 45e9d58 into main Apr 4, 2025
19 of 20 checks passed
@bgn42 bgn42 deleted the issue_480 branch April 4, 2025 15:23
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.

3 participants