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

Need to update source exclusions for ICU 61 #16937

Closed
srl295 opened this issue Nov 10, 2017 · 6 comments
Closed

Need to update source exclusions for ICU 61 #16937

srl295 opened this issue Nov 10, 2017 · 6 comments
Assignees
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. i18n-api Issues and PRs related to the i18n implementation.

Comments

@srl295
Copy link
Member

srl295 commented Nov 10, 2017

  • Platform: all
  • Subsystem: deps

#16876 landed ICU 60.1 but didn't update the source exclusions in icu_generic.gyp. As a result:

  • too many files are built- increases binary size, and source size (this also controls what is checked into the repo)
  • there's more exposure for platform build issues (probably a minor point)

This would cause NO change in actual behavior, because the files aren't actually executed (or their removal would have caused the build to fail).

@srl295 srl295 self-assigned this Nov 10, 2017
@srl295
Copy link
Member Author

srl295 commented Nov 10, 2017

should have read the guide in #7843 !

@srl295
Copy link
Member Author

srl295 commented Nov 10, 2017

OK, looking at the exclusions: I only put them in for 55 and 57 (copy and paste). So they are missing for 56(#3281), 58 (#9234/#10206), 59 (#12486) AND 60 (#16876). So this is not a new issue.

Also fixing this now would break the floating patch #16931 without some care. (it could be re-applied)

Eminently back-port-able.

Instead of copy and paste, a range should be used for exclusions.

@srl295 srl295 added c++ Issues and PRs that require attention from people who are familiar with C++. i18n-api Issues and PRs related to the i18n implementation. labels Nov 10, 2017
@srl295
Copy link
Member Author

srl295 commented Jan 30, 2018

^ floating patch mentioned above is rolled into the #17687 so shouldn't be a problem.

@apapirovski
Copy link
Member

@srl295 was this fixed when #17687 landed?

@srl295 srl295 changed the title Need to update source exclusions for ICU 60 Need to update source exclusions for ICU 61 Apr 13, 2018
@srl295
Copy link
Member Author

srl295 commented Apr 13, 2018

@apapirovski no, this a minor improvement is to add some more exclusions. It was unblocked by the above. Should now be 61 due to #19621

@jasnell
Copy link
Member

jasnell commented Aug 11, 2018

@srl295 ... still needed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. i18n-api Issues and PRs related to the i18n implementation.
Projects
None yet
Development

No branches or pull requests

3 participants