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

Intl: prepare for ICU 56 bump #3065

Closed
srl295 opened this issue Sep 25, 2015 · 3 comments
Closed

Intl: prepare for ICU 56 bump #3065

srl295 opened this issue Sep 25, 2015 · 3 comments
Assignees
@srl295
Labels
i18n-api Issues and PRs related to the i18n implementation.

Comments

@srl295
Copy link
Member

srl295 commented Sep 25, 2015

Even if we don't yet make ICU 56 default as in #2917 I'd like to make it so that people who manually choose 56 won't get a failure.

A file was renamed from derb.c to derb.cpp and so currently a build issue results in this file not being properly excluded. Since it's not explicitly built, it's fine to just list both the .c and .cpp versions in the exclude list.
@srl295 srl295 self-assigned this Sep 25, 2015
@srl295 srl295 added the i18n-api Issues and PRs related to the i18n implementation. label Sep 25, 2015
@srl295
Copy link
Member Author

srl295 commented Sep 25, 2015

(by the way, this was caught by the continuous integration of ICU-trunk with Node-master, as discussed at the collaborator's summit.)

srl295 added a commit to srl295/node-v0.x-archive that referenced this issue Sep 25, 2015
@srl295
build: Intl: avoid 'duplicate main()' on ICU 56
4e86f2d 
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/node#3065
@srl295 srl295 mentioned this issue Sep 25, 2015
Closed
@srl295
Copy link
Member Author

srl295 commented Sep 25, 2015

@mhdawson fyi

@srl295 srl295 mentioned this issue Sep 25, 2015
Closed
@srl295 srl295 closed this as completed in aa97ae7 Oct 6, 2015
@srl295
Copy link
Member Author

srl295 commented Oct 7, 2015

Unfortunately this fix was incomplete. It only works for the full-icu case. For small-icu it missed some other .c to .cpp changes.

Given the time, just fix the rest of the issues in #2917

Looks like a false alarm / unclean local build dir. Clean build worked fine.

srl295 added a commit that referenced this issue Oct 8, 2015
@srl295 @jasnell
build: intl: avoid 'duplicate main()' on ICU 56
f00f326 
* 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
This was referenced Sep 17, 2021
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@srl295 srl295

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

No branches or pull requests
1 participant
@srl295