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

plugins: manual_download: Resolve copyFile promise #1786

Merged
merged 2 commits into from Mar 1, 2021
Merged

plugins: manual_download: Resolve copyFile promise #1786

merged 2 commits into from Mar 1, 2021

Conversation

ix5
Copy link
Contributor

@ix5 ix5 commented Feb 7, 2021

The whole block beginning with .then(downloadedFilePath and ending with copyFile() (^1) was never going to resolve its promise with an actual return value.
That meant that that checkFile() was being called before any of the promise functions inside the previous block had
the chance to run, therefore the copied file was not yet available, leading to checkFile trying to verify a non-existent path, leading to a checksum mismatch error.

Solution is to make the whole block return something, in this case the value of the last chained promise.

Solves:

error: Error: core:manual_download: Error: checksum mismatch

Fixes #1754

^1:

.then(downloadedFilePath => {
fs.ensureDir(
path.join(this.cachePath, this.props.config.codename, group)
).then(() =>
fs.copyFile(
downloadedFilePath,
path.join(
this.cachePath,
this.props.config.codename,
group,
file.name
)
)
);
})

@ix5 ix5 requested a review from NeoTheThird as a code owner February 7, 2021 19:08
@codecov-io
Copy link

codecov-io commented Feb 7, 2021

Codecov Report

Merging #1786 (974468a) into master (95745e5) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1786   +/-   ##
=======================================
  Coverage   57.62%   57.62%           
=======================================
  Files          26       26           
  Lines         800      800           
=======================================
  Hits          461      461           
  Misses        339      339           
Impacted Files Coverage Δ
src/core/plugins/core/plugin.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95745e5...974468a. Read the comment docs.

@ix5
Copy link
Contributor Author

ix5 commented Feb 7, 2021

Pushed the lint fix in a separate commit

The whole block beginning with `.then(downloadedFilePath`
and ending with `copyFile()` did not return anything and was
never going to resolve its promise with an actual return
value.
That meant that that `checkFile()` was being called before
any of the promise functions inside the previous block had
the chance to run, therefore the copied file was not yet
available, leading to checkFile trying to verify a
non-existent path, leading to a checksum mismatch error.

Solution is to make the whole block return something, in
this case the value of the last chained promise, by removing
the curly braces.

Solves:
```
error: Error: core:manual_download: Error: checksum mismatch
```

Fixes #1754
Generated by `npm run lint-fix`
@ix5
Copy link
Contributor Author

ix5 commented Feb 12, 2021

@NeoTheThird @UniversalSuperBox mind taking a look at this?

@NeoTheThird NeoTheThird self-assigned this Feb 17, 2021
@ix5
Copy link
Contributor Author

ix5 commented Feb 21, 2021

Since Jan seems MIA currently, @Flohack74 would you like to review this?

Copy link
Member

@NeoTheThird NeoTheThird left a comment

Choose a reason for hiding this comment

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

lgtm, sorry for the delay and thanks for the contribution

@NeoTheThird NeoTheThird merged commit 9447356 into ubports:master Mar 1, 2021
@ix5 ix5 deleted the manual-dl-resolve-promise branch March 1, 2021 16:43
@ix5
Copy link
Contributor Author

ix5 commented Mar 1, 2021

lgtm, sorry for the delay and thanks for the contribution

Hey, you're under no obligation here. Thanks for writing this installer in the first place!

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.

Sony Xperia X reports checksum error of image file in ubports installer 0.8.4
4 participants