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

Improved tools checksums (EIM-109) #48

Merged
merged 1 commit into from
Feb 10, 2025

Conversation

Hahihula
Copy link
Collaborator

@Hahihula Hahihula commented Feb 4, 2025

image
image

@Hahihula Hahihula force-pushed the EIM-109-improved-sha-verifgication branch from 6648dd5 to e69a175 Compare February 4, 2025 13:17
@brianignacio5
Copy link

brianignacio5 commented Feb 6, 2025

When i tested using the expert setup it hangs on IDF Cmake when using dl.espressif.cn mirror:

Screenshot 2025-02-06 183851

The setup fails to check prerequisites in MacOS M1 (ninja, cmake dfu-util).

@@ -31,6 +31,7 @@
<tr data-id="tools-table-header">
<th>Tool</th>
<th>Downloaded</th>
<th>SHA</th>
Copy link
Collaborator

Choose a reason for hiding this comment

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

SHA is used internally to verify the downloaded file. Do we really need to show this to the user?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

how do you suggest informing the user when the checksum verification fails after we removed the popup messages?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We could consider combining the “Downloaded” status with “SHA” verification for a more concise status update. If the file is downloaded but does not match the checksum, we can mark it as an invalid file and display an error flag.

Exposing too many internal details with different labels might unnecessarily complicate the UI, especially when the user cannot take any specific action based on them.

Think about it.

Copy link

@brianignacio5 brianignacio5 left a comment

Choose a reason for hiding this comment

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

LGTM

@Hahihula Hahihula merged commit fc795bc into master Feb 10, 2025
5 of 10 checks passed
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