doc: Minor fix on ICU maintenance, and fix in autogenerated readme #32347
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Drafting this because I'll work on an ICU 66.1 update and might hit something else |
|
OK, I remember what the python fix is for: python3. works in python2, but 3 says: srl@pinguino:~/src/node$ python tools/icu/shrink-icu-src.py
Deleting existing icudst deps/icu-small
Data file root: icudt66l
will use datafile deps/icu/source/data/in/icudt66l.dat
deps/icu --> deps/icu-small
27M deps/icu/source/data/in/icudt66l.dat
deps/icu/source/data/in/icudt66l.dat --compress-> deps/icu-small/source/data/in/icudt66l.dat.bz2
9M deps/icu-small/source/data/in/icudt66l.dat.bz2
Traceback (most recent call last):
File "tools/icu/shrink-icu-src.py", line 132, in <module>
print("ICU sources - auto generated by shrink-icu-src.py", file=fi)
TypeError: a bytes-like object is required, not 'str' |
Closed
|
By 'dead code' I mean the configure logic to automatically download ICU, because it's already there unless you delete deps/icu-small. We should keep the downloader itself, because it is needed for the But the embedded download URL, and its md5 hash, and the logic for verifying the download, doesn't seem needed anymore. |
ryzokuken
approved these changes
Mar 18, 2020
jasnell
approved these changes
Mar 18, 2020
richardlau
approved these changes
Mar 18, 2020
|
cc @nodejs/python |
cclauss
reviewed
Mar 19, 2020
tools/configure.d/nodedownload.py
Outdated
| @@ -63,7 +63,7 @@ def checkHash(targetfile, hashAlgo): | |||
| digest = hashlib.new(hashAlgo) | |||
| with open(targetfile, 'rb') as f: | |||
| chunk = f.read(1024) | |||
| while chunk != "": | |||
| while len(chunk) > 0: | |||
There was a problem hiding this comment.
Why not just...
Suggested change
| while len(chunk) > 0: | |
| while chunk: |
|
#31659 Landed which touched the same file. @bioinfornatics |
|
@cclauss @bioinfornatics the other fix works, so i removed icutrim from this PR… |
cclauss
approved these changes
Mar 19, 2020
cclauss
reviewed
Mar 19, 2020
srl295
added
the
python
|
Landed in b8a794d |
srl295
added a commit
that referenced
this pull request
Mar 21, 2020
- Docs used the word "copy" when it really meant a tool is needed. - README-FULL-ICU.txt was generated in binary mode, but it's a text file. This breaks on Python3 for maintaining ICU - The ICU downloader was broken (also probably python3). It's basically dead code since 1a25e90 landed (full icu in repo), unless someone deleted the deps/icu-small directory from their repo. Co-Authored-By: Christian Clauss <[email protected]> PR-URL: #32347 Reviewed-By: Ujjwal Sharma <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Christian Clauss <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
MylesBorins
pushed a commit
that referenced
this pull request
Mar 24, 2020
- Docs used the word "copy" when it really meant a tool is needed. - README-FULL-ICU.txt was generated in binary mode, but it's a text file. This breaks on Python3 for maintaining ICU - The ICU downloader was broken (also probably python3). It's basically dead code since 1a25e90 landed (full icu in repo), unless someone deleted the deps/icu-small directory from their repo. Co-Authored-By: Christian Clauss <[email protected]> PR-URL: #32347 Reviewed-By: Ujjwal Sharma <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Christian Clauss <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Docs used the word "copy" when it really meant a tool is needed.
README-FULL-ICU.txt was generated in binary mode, but it's a
text file. This breaks on Python3 for maintaining ICU
The ICU downloader was broken (also probably python3). It's
basically dead code since 1a25e90
landed (full icu in repo), unless someone deleted the deps/icu-small
directory from their repo.
For that matter, small-icu (icutrim) was also broken on python3,Fixed in fix #31650 right use of string and bytes objects #31659it was not excluding what it ought to, so 'small' was about 9M instead
of 3M.
documentation is changed or added
commit message follows commit guidelines