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

Update README.md #350

Closed
wants to merge 1 commit into from
Closed

Conversation

marcelinho21
Copy link

@marcelinho21 marcelinho21 commented Mar 16, 2023

Fix for Windows OS guide to verify SeedSigner OS binaries

Fix for Windows OS to verify Binaries
```
CertUtil -hashfile seedsigner.0.6.0.sha256 SHA256 | findstr /v "hash"
seedsigner_os.0.6.x[].img: OK
Copy link
Contributor

Choose a reason for hiding this comment

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

ack

@@ -201,19 +201,19 @@ Now that you have confirmed that you do have the real SeedSigner Project's Publi
```
shasum -a 256 --ignore-missing --check seedsigner.0.6.*.sha256
```

**On Windows (inside Powershell):** Run this command
Wait up to 30 seconds for the command to complete, and it should display:
Copy link
Contributor

Choose a reason for hiding this comment

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

this line can now be removed as the 0.6.x hash calculation is instant. See my comment below on moving downwards, however .

```
seedsigner_os.0.6.x[Your_Pi_Model_For_Example:pi02w].img: OK
CertUtil -hashfile seedsigner_os.0.6.0.[Your_Pi_Model_For_Example:pi02w].img SHA256 | findstr /v "hash"
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect the reason for this PR moving the OSX/Linux "result" section upwards is becuasethe windows section is longer and it is not clear where the Linux section ends and which step is thius next. (We have limited formatting options in Markdown. not even font colors.)

however, there is going to be another PR created in the next few days to make the complete windows hash value checking a single command. without the current 2nd manual step. and so almost like the shamsum command.

So, I suggest to leave this windows command's vertical position unchanged until we have that replacement windows PR merged.

It would also be inconsistent with the other document sections that do the OS commands, and then show the common result description and next steps.
I do agree however that windows is looking like the poor cousin here, but I have draft code using the modern GetFileHash to correct that.

@Marc-Gee
Copy link
Contributor

Marc-Gee commented Mar 17, 2023

Marcel , thank you very much for your PR. Its does correctly note that the OSX/Linux and Windows sections do not indicate their conclusion very clearly and I agree that it is confusing where to go next. Our formatting options in MD lang are very limited unfortunately.

My concern with the change overall is that It would also be inconsistent with the other document sections that do the differnt OS commands, and thereafter show the common result description and next steps. combined to keep the readme as short as possible.

I do agree however that windows is looking like the poor cousin here, but I have draft code using the modern GetFileHash to correct that.
there is a fix for the extra dot on line 210, in the existing PR346.
And which will now incorporate the error discovered by Wisam in the TG on March 15 2023. (We are hashing the incorrect file).

Perhaps you will participate in the review of that revised layout when its up for review in a few days time?

Marc.

@Marc-Gee
Copy link
Contributor

@marcelinho21 can you please weigh in on #352 which I just submitted.
I believe it will cover your PR and will catch-up our windows users to to OSX/Linux. since its a single step for both , the readme document subsequent step will flow-on better now.

The new powershell command could be made into a single, long 1-liner, but I would be hesitant to run script I dont somewhat understand on my own machine. (But of course I definately have done that , in a frustrated moment! 😉)
MarcG

@newtonick newtonick added merge conflicts has merge conflicts that need resolution documentation Improvements or additions to documentation labels Aug 1, 2023
@marc3linho
Copy link

@SeedSigner can you please close this PR. I no longer have access to this account.
The topic is already covered in #352

@SeedSigner SeedSigner closed this Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation merge conflicts has merge conflicts that need resolution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants