search

electron / get

Download Electron release artifacts
https://npm.im/@electron/get
MIT License
338 stars 106 forks source link

fix!: download electron checksum failure #226

Closed BlackHole1 closed 2 years ago

BlackHole1 commented 2 years ago

Bug context see: https://github.com/electron/fiddle/issues/1156

This bug is only triggered in the electron-renderer and when Webpack is packaged.

Break Changes

  1. Node.js version must >= 12.0.0
BlackHole1 commented 2 years ago

CI failed reason: Cannot find a definition for job named test-linux-12. cc @malept

BlackHole1 commented 2 years ago

It may take time to explain this matter from beginning to end, so please wait. In the meantime, you can start by checking out: https://github.com/electron/fiddle/issues/1156

BlackHole1 commented 2 years ago

Tips

  1. It is difficult to write the corresponding unit tests in electron/get. Because the problem only occurs in the electron-renderer process and uses the Webpack build. This can be reproduced at https://github.com/electron/fiddle/releases/tag/v0.29.2 (Select China Mirrors)
  2. This bug is fixed at https://github.com/sindresorhus/got/pull/1105
  3. There is a discussion of this bug in got. https://github.com/sindresorhus/got/issues/945

Why update got

Yesterday, I found that my electron-fiddle was not downloading electron, and after some troubleshooting, I figured out that it was not parsing gzip when downloading with got.stream. So when @electron/get uses got.stream to download SHASUMS256.txt, the response returns gzip, which causes @electron/get to fail the checksum.

https://github.com/electron/get/blob/5c81f9a388577a9d446b2f7ae1a6e2dd2d7177d6/src/index.ts#L131-L141

https://github.com/electron/get/blob/5c81f9a388577a9d446b2f7ae1a6e2dd2d7177d6/src/index.ts#L151-L153

https://github.com/electron/get/blob/5c81f9a388577a9d446b2f7ae1a6e2dd2d7177d6/src/GotDownloader.ts#L60

This seems like a pretty large change for a single bug.

We talked about "no longer supporting node.js < 10" once before. See: https://github.com/electron/get/pull/214#discussion_r804004094

Because we have other problems caused by the low version of got https://github.com/electron/get/issues/224

I think this is the time to do it.

malept commented 2 years ago

It would be helpful if you could split out the changes into separate logical commits so that it's easier to review.

BlackHole1 commented 2 years ago

It would be helpful if you could split out the changes into separate logical commits so that it's easier to review.

Sorry, I didn't quite understand it. This PR is already a minimal change and cannot be split any further

malept commented 2 years ago

It would be helpful if you could split out the changes into separate logical commits so that it's easier to review.

Sorry, I didn't quite understand it. This PR is already a minimal change and cannot be split any further

You can start with #225 and build upon it.

BlackHole1 commented 2 years ago

OK, I will close this PR as there are duplicate PRs