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

deps: fix icu-shrinker to handle ICU 58 #8674

Closed
wants to merge 1 commit into from

Conversation

srl295
Copy link
Member

@srl295 srl295 commented Sep 20, 2016

  • commit message follows commit guidelines
Affected core subsystem(s)
  • deps
Description of change

ICU’s license file changed from license.html to LICENSE.
Fix shrink-icu-src.py (which is manually run) to accept either file.

Related to #7844 which is the 58 bump.

ICU’s license file changed from `license.html` to `LICENSE`.
Fix `shrink-icu-src.py` (which is manually run) to accept either file.

Related to nodejs#7844 which is the 58 bump.
@srl295 srl295 added tools Issues and PRs related to the tools directory. i18n-api Issues and PRs related to the i18n implementation. labels Sep 20, 2016
@srl295 srl295 self-assigned this Sep 20, 2016
@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Sep 20, 2016
if 'license.html' in ign:
ign.remove('license.html')
if 'LICENSE' in ign:
ign.remove('LICENSE')
Copy link
Member

Choose a reason for hiding this comment

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

out of my head but perhaps if any(item in ign for item in('LICENSE', 'license.html'))

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. I'm going to change the logic a bit here. We want to keep 'LICENSE' || 'license.html' in that order.

elif subdir == 'source/data':
ign = ign + ['unidata','curr','zone','unit','lang','region','misc','sprep']
#elif subdir == 'source/data':
# ign = ign + ['unidata','curr','zone','unit','lang','region','misc','sprep']
# else:
Copy link
Member

Choose a reason for hiding this comment

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

remove?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, will remove. and the following comments.

@rvagg rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02
@srl295
Copy link
Member Author

srl295 commented Oct 25, 2016

@jbergstroem I'm not sure where I ended up with this… but I'll revisit your comments now that #7844 is now in progress.

@srl295 srl295 mentioned this pull request Oct 25, 2016
4 tasks
@srl295
Copy link
Member Author

srl295 commented Oct 25, 2016

@jbergstroem I'm going to fold this into #9234 and close this PR.

@srl295 srl295 closed this Oct 25, 2016
@srl295 srl295 deleted the fix-icu-shrinker branch October 25, 2016 18:25
srl295 added a commit to srl295/node that referenced this pull request Oct 31, 2016
* bump to ICU 58.1 - update URL / hash
* does not attempt to reduce size - yet
* patch to work around http://bugs.icu-project.org/trac/ticket/12822
  ( compile issue on Windows)
* Fix ICU shrinker to delete old license.html file
  (update to nodejs#8674 )

Fixes: nodejs#7844
PR-URL: nodejs#9234
Reviewed-By: James M Snell <jasnell@gmail.com>
srl295 added a commit that referenced this pull request Oct 31, 2016
* bump to ICU 58.1 - update URL / hash
* does not attempt to reduce size - yet
* patch to work around http://bugs.icu-project.org/trac/ticket/12822
  ( compile issue on Windows)
* Fix ICU shrinker to delete old license.html file
  (update to #8674 )

Fixes: #7844
PR-URL: #9234
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request Nov 3, 2016
* bump to ICU 58.1 - update URL / hash
* does not attempt to reduce size - yet
* patch to work around http://bugs.icu-project.org/trac/ticket/12822
  ( compile issue on Windows)
* Fix ICU shrinker to delete old license.html file
  (update to #8674 )

Fixes: #7844
PR-URL: #9234
Reviewed-By: James M Snell <jasnell@gmail.com>
srl295 added a commit to srl295/node that referenced this pull request Jan 18, 2017
* bump to ICU 58.1 - update URL / hash
* does not attempt to reduce size - yet
* patch to work around http://bugs.icu-project.org/trac/ticket/12822
  ( compile issue on Windows)
* Fix ICU shrinker to delete old license.html file
  (update to nodejs#8674 )

Fixes: nodejs#7844
PR-URL: nodejs#9234
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 20, 2017
* bump to ICU 58.1 - update URL / hash
* does not attempt to reduce size - yet
* patch to work around http://bugs.icu-project.org/trac/ticket/12822
  ( compile issue on Windows)
* Fix ICU shrinker to delete old license.html file
  (update to #8674 )

Fixes: #7844
PR-URL: #9234
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
* bump to ICU 58.1 - update URL / hash
* does not attempt to reduce size - yet
* patch to work around http://bugs.icu-project.org/trac/ticket/12822
  ( compile issue on Windows)
* Fix ICU shrinker to delete old license.html file
  (update to #8674 )

Fixes: #7844
PR-URL: #9234
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
* bump to ICU 58.1 - update URL / hash
* does not attempt to reduce size - yet
* patch to work around http://bugs.icu-project.org/trac/ticket/12822
  ( compile issue on Windows)
* Fix ICU shrinker to delete old license.html file
  (update to #8674 )

Fixes: #7844
PR-URL: #9234
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
i18n-api Issues and PRs related to the i18n implementation. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants