Skip to content

Support for icu4c 68.1 #6397

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

Closed
wants to merge 1 commit into from
Closed

Conversation

derrabus
Copy link
Contributor

@derrabus derrabus commented Nov 3, 2020

@cmb69
Copy link
Member

cmb69 commented Nov 3, 2020

Thanks for the PR! However, for the stable branches (7.3 and 7.4) I'd prefer to define U_DEFINE_FALSE_AND_TRUE (either via config.m4 or in the sources before any ICU header is included). For 8.0/master I'd prefer to no longer use TRUE and FALSE at all.

@sgolemon, what do you think?

@derrabus
Copy link
Contributor Author

derrabus commented Nov 3, 2020

However, for the stable branches (7.3 and 7.4) I'd prefer to define U_DEFINE_FALSE_AND_TRUE

All right, I'll try that.

For 8.0/master I'd prefer to no longer use TRUE and FALSE at all.

What would be the alternative? Use 1 and 0 literals instead?

@derrabus
Copy link
Contributor Author

derrabus commented Nov 3, 2020

I've added the setting to PHP_SETUP_ICU in acinclude.m4. Is that the right spot?

@cmb69
Copy link
Member

cmb69 commented Nov 9, 2020

What would be the alternative? Use 1 and 0 literals instead?

We're requiring C99 for 8.0/master anyway, so we could use true and false.

I've added the setting to PHP_SETUP_ICU in acinclude.m4. Is that the right spot?

Not quite sure. I'm mostly working on Windows, and there, TRUE and FALSE are already defined by some Windows header file, and as such ICU 68.1 appears to run fine without any changes.

Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me (for stable branches).

@php-pulls php-pulls closed this in 8eaaabd Nov 9, 2020
@derrabus derrabus deleted the bugfix/icu4c-68.1 branch November 9, 2020 13:40
@derrabus
Copy link
Contributor Author

derrabus commented Nov 9, 2020

Thank you for merging.

We're requiring C99 for 8.0/master anyway, so we could use true and false.

I see. I'll work on a PR when this change has been merged to PHP-8.0.

@cmb69
Copy link
Member

cmb69 commented Nov 9, 2020

I see. I'll work on a PR when this change has been merged to PHP-8.0.

Nikita already did that. :)

Since this PR has been merged into PHP-7.3, it may make sense to backport 77b6e95 as well.

@ghost
Copy link

ghost commented Mar 5, 2021

If you have the time, could you please backport PHP 7.2 ?

@derrabus
Copy link
Contributor Author

derrabus commented Mar 5, 2021

@Taki PHP 7.2 is EOL and won't be patched anymore. But you can simply set those CFLAGS locally yourself when compiling PHP 7.2 and it should work. Homebrew for instance does just that:

https://github.com/Homebrew/homebrew-core/blob/8c20c466845d263b9d63ceab484805eb86136268/Formula/php%407.2.rb#L67-L69

@Echecivuole
Copy link

Hello all, I have same issue and Abel to fix the icu4c 68.2 or revert to 67.1. Any help?

I understood I have to add this -> ICU_CFLAGS="$ICU_CFLAGS -DU_DEFINE_FALSE_AND_TRUE=1", but not clear where

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants