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.

Sign up for GitHub

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

Jump to bottom

build: allow icu download to use sha512 over md5 #27370

Closed
wants to merge 0 commits into from
Closed

build: allow icu download to use sha512 over md5 #27370

wants to merge 0 commits into from

Conversation

srl295
Copy link
Member

@srl295 srl295 commented Apr 23, 2019 •
edited
Loading

  • ICU uses sha512 instead of md5 in some recent releases
  • Use hashlib.algorithms_guaranteed to choose the following algorithms:
    sha1 sha224 sha384 sha256 sha512 md5
  • No preference as to the priority of the algorithms
  • This commit does not change the hash used for ICU.

Fixes: #27369

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@srl295 srl295 added the i18n-api Issues and PRs related to the i18n implementation. label Apr 23, 2019
@srl295 srl295 requested review from refack and ryzokuken April 23, 2019 17:04
@srl295 srl295 self-assigned this Apr 23, 2019
@nodejs-github-bot
Copy link
Collaborator

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3341

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Apr 23, 2019
@srl295
Copy link
Member Author

srl295 commented Apr 23, 2019

Example current_ver.dep:

[
  {
    "url": "https://github.com/unicode-org/icu/releases/download/release-63-1/icu4c-63_1-src.zip",
    "sha512": "9c42ff474834836c32f288ca26f2f8c9674bee24d46a8d85c9bc1e8c965db3a3a93fe57598acf3839abc8ca0576f0d9211be8d235979607f12d425d2ce46c3cc"
  },
  {
    "url": "https://sourceforge.net/projects/icu/files/ICU4C/63.1/icu4c-63_1-src.zip",
    "sha512": "9c42ff474834836c32f288ca26f2f8c9674bee24d46a8d85c9bc1e8c965db3a3a93fe57598acf3839abc8ca0576f0d9211be8d235979607f12d425d2ce46c3cc"
  },
  {
    "url": "https://download.icu-project.org/files/icu4c/63.1/icu4c-63_1-src.zip",
    "sha512": "9c42ff474834836c32f288ca26f2f8c9674bee24d46a8d85c9bc1e8c965db3a3a93fe57598acf3839abc8ca0576f0d9211be8d235979607f12d425d2ce46c3cc"
  }
]

If no hashes are in the file you get:

ERROR: Could not find a hash to verify ICU download.
          tools/icu/current_ver.dep may be incorrect.
          For the entry https://github.com/unicode-org/icu/releases/download/release-63-1/icu4c-63_1-src.zip,
          Expected one of these keys: sha1 sha224 sha384 sha256 sha512 md5

@srl295
Copy link
Member Author

srl295 commented Apr 23, 2019 •
edited
Loading

idea from #27361 - I want to update the docs to say that sha512 could be used, but want to wait for that to land first.

ryzokuken
ryzokuken approved these changes Apr 23, 2019
View reviewed changes
Copy link
Contributor

@ryzokuken ryzokuken left a comment

Choose a reason for hiding this comment

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

Thanks for this!

srl295
srl295 commented Apr 23, 2019
View reviewed changes
tools/configure.d/nodedownload.py Outdated
# error
return (None, None, availAlgos)

def checkHash(targetfile, hashAlgo):
"""md5sum a file. Return the hex digest."""
Copy link
Member Author

Choose a reason for hiding this comment

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

oops

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@srl295 srl295 force-pushed the icu-sha512 branch 2 times, most recently from b8eb88a to a461aa3 Compare April 24, 2019 21:44
@srl295 srl295 requested review from ryzokuken, BridgeAR and richardlau and removed request for ryzokuken and BridgeAR April 24, 2019 21:44
@srl295
Copy link
Member Author

srl295 commented Apr 24, 2019

fixed commit message and python docs…

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/22696/

@srl295 srl295 mentioned this pull request Apr 26, 2019
Closed
@targos
Copy link
Member

targos commented Apr 26, 2019

@srl295 You pushed this commit to master without review metadata. It's still possible to force-push to fix this.

@srl295
Copy link
Member Author

srl295 commented Apr 26, 2019 •
edited
Loading

@targos yeah, i just did that. … got my process wrong … 

srl295 added a commit that referenced this pull request Apr 26, 2019
@srl295
build: allow icu download to use other hashes besides md5
d04b376 
- ICU uses sha512 instead of md5 in some recent releases
- Use hashlib.algorithms_guaranteed to choose the following algorithms:
    sha1 sha224 sha384 sha256 sha512 md5
- No preference as to the priority of the algorithms
- This commit does not change the hash used for ICU.

Fixes: #27369
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-by: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-by: Richard Lau <riclau@uk.ibm.com>
PR-URL: #27370
@srl295 srl295 closed this Apr 26, 2019
@srl295 srl295 deleted the icu-sha512 branch April 26, 2019 15:28
@srl295
Copy link
Member Author

srl295 commented Apr 26, 2019

Landed in d04b376

targos pushed a commit that referenced this pull request Apr 27, 2019
@srl295 @targos
build: allow icu download to use other hashes besides md5
e7026f1 
- ICU uses sha512 instead of md5 in some recent releases
- Use hashlib.algorithms_guaranteed to choose the following algorithms:
    sha1 sha224 sha384 sha256 sha512 md5
- No preference as to the priority of the algorithms
- This commit does not change the hash used for ICU.

Fixes: #27369
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-by: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-by: Richard Lau <riclau@uk.ibm.com>
PR-URL: #27370
@targos targos mentioned this pull request Apr 27, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@refack refack refack approved these changes

@richardlau richardlau richardlau approved these changes

@ryzokuken ryzokuken Awaiting requested review from ryzokuken

@BridgeAR BridgeAR Awaiting requested review from BridgeAR

Assignees

@srl295 srl295

Labels
build Issues and PRs related to build files or the CI. i18n-api Issues and PRs related to the i18n implementation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

tools: support sha512 in the ICU current_ver.dep
7 participants
@srl295 @nodejs-github-bot @targos @refack @richardlau @BridgeAR @ryzokuken