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

build: Intl: avoid 'duplicate main()' on ICU 56 #3066

Closed
wants to merge 1 commit into from

Conversation

srl295
Copy link
Member

@srl295 srl295 commented Sep 25, 2015

ICU 56 renamed derb.c to derb.cpp because of C++ yay.
This broke the exclusion of "derb.c" when building tools.

Solution is to add derb.c AND derb.cpp to exclusion.
We don't build the 'derb' tool, so it's fine to list the
excluded source twice.

Reviewed-By:
PR-URL:
Fixes: #3065

ICU 56 renamed derb.c to derb.cpp because of C++ yay.
This broke the exclusion of "derb.c" when building tools.

Solution is to add derb.c AND derb.cpp to exclusion.
We don't build the 'derb' tool, so it's fine to list the
excluded source twice.

Reviewed-By:
PR-URL:
Fixes: nodejs#3065
@srl295 srl295 added the i18n-api Issues and PRs related to the i18n implementation. label Sep 25, 2015
@srl295 srl295 self-assigned this Sep 25, 2015
@bnoordhuis
Copy link
Member

LGTM. Is there a reason icu_src_derb is a variable? It's only used in one place, right?

@srl295
Copy link
Member Author

srl295 commented Sep 25, 2015

@bnoordhuis It's only used one place. All of the other variables are generated by inspection of the ICU source base. ( there's an icu_src_common etc. auto generated with all files in icu/source/common). I was going for parallelism there.

BTW ICU 56 rc1 dropped today.

@srl295
Copy link
Member Author

srl295 commented Oct 6, 2015

@srl295 srl295 removed their assignment Oct 6, 2015
srl295 added a commit that referenced this pull request Oct 6, 2015
* Exclude `derb.cpp` as well as `derb.c` from Node builds
  (file was renamed in ICU 56)

ICU 56 renamed derb.c to derb.cpp because of C++ yay.
This broke the exclusion of "derb.c" when building tools.

Solution is to add derb.c AND derb.cpp to exclusion.
We don't build the 'derb' tool, so it's fine to list the
excluded source twice.

Reviewed-By: Ben Noordhuis <[email protected]>
PR-URL: #3066
Fixes: #3065
@srl295 srl295 closed this Oct 6, 2015
@jasnell
Copy link
Member

jasnell commented Oct 7, 2015

LGTM. Would be good to get ICU56 into v4.x given the overall performance improvements.

@jasnell jasnell mentioned this pull request Oct 8, 2015
29 tasks
srl295 added a commit that referenced this pull request Oct 8, 2015
* Exclude `derb.cpp` as well as `derb.c` from Node builds
  (file was renamed in ICU 56)

ICU 56 renamed derb.c to derb.cpp because of C++ yay.
This broke the exclusion of "derb.c" when building tools.

Solution is to add derb.c AND derb.cpp to exclusion.
We don't build the 'derb' tool, so it's fine to list the
excluded source twice.

Reviewed-By: Ben Noordhuis <[email protected]>
PR-URL: #3066
Fixes: #3065
@MylesBorins
Copy link
Contributor

landed in v4.x-staging as f00f326

@srl295 srl295 deleted the icu-55-3065 branch July 6, 2018 21:01
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants