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

icu4c 61.1 #25799

Closed
wants to merge 29 commits into from
Closed

icu4c 61.1 #25799

wants to merge 29 commits into from

Conversation

commitay
Copy link
Contributor

Created with brew bump-formula-pr.

@SMillerDev
Copy link
Member

All of the failing projects are probably affected by http://site.icu-project.org/download/61#TOC-Migration-Issues

@SMillerDev
Copy link
Member

PHP issue reported upstream here: https://bugs.php.net/bug.php?id=76153

@commitay
Copy link
Contributor Author

Thanks @SMillerDev!

@ilovezfs
Copy link
Contributor

We can add ENV.append "CPPFLAGS", "-DU_USING_ICU_NAMESPACE=1" to each of the formulae that broke as long as the breakage also gets reported to each upstream.

@ilovezfs
Copy link
Contributor

For chakra --extra-defines=U_USING_ICU_NAMESPACE=1

@ilovezfs
Copy link
Contributor

For mapnik

diff --git a/Formula/mapnik.rb b/Formula/mapnik.rb
index aba19d3..7705656 100644
--- a/Formula/mapnik.rb
+++ b/Formula/mapnik.rb
@@ -50,6 +50,7 @@ class Mapnik < Formula
             "CXX=\"#{ENV.cxx}\"",
             "PREFIX=#{prefix}",
             "CUSTOM_CXXFLAGS=\"-DBOOST_EXCEPTION_DISABLE\"",
+            "CUSTOM_DEFINES=-DU_USING_ICU_NAMESPACE=1",
             "ICU_INCLUDES=#{icu}/include",
             "ICU_LIBS=#{icu}/lib",
             "JPEG_INCLUDES=#{jpeg}/include",

@ilovezfs
Copy link
Contributor

For widelands ENV.append "CXXFLAGS", "-DU_USING_ICU_NAMESPACE=1"

@chrmoritz
Copy link
Contributor

For node the issue was fixed in nodejs/node@b8f47b2. But the issue with node@8 is somehow expected, because every node version officially only support the icu version it's ships with, which is icu 60 for node@8. So we either have to change the formula to build against the bundled icu again, build it with -DU_USING_ICU_NAMESPACE=1 or apply said patch.

@ilovezfs
Copy link
Contributor

For zorba, also ENV.append "CXXFLAGS", "-DU_USING_ICU_NAMESPACE=1"

@ilovezfs
Copy link
Contributor

@chrmoritz why would upstream refuse to apply backwards compatible fixes to the 8.x branch? I don't think we should assume that.

@ilovezfs
Copy link
Contributor

@srl295 any chance of getting nodejs/node@b8f47b2 back ported to Node 8.x LTS?

@srl295
Copy link
Contributor

srl295 commented Mar 28, 2018

@chrmoritz

every node version officially only support the icu version it's ships with, which is icu 60 for node@8

That's not officially true (officially not true?)… I opened nodejs/node#19624 2 days ago in master explicitly to support ICU 58.2. Actually I had opened nodejs/Intl#35 ( now nodejs/node#19657 ) to explicitly document which versions are supported, but have not done it yet. I'll try to at least document something.

@ilovezfs

@srl295 any chance of getting nodejs/node@b8f47b2 back ported to Node 8.x LTS?

You've done 90% of the work already (asking, identifying the commit). Yes, that's a great idea.

We can add ENV.append "CPPFLAGS", "-DU_USING_ICU_NAMESPACE=1" to each of the formulae that broke as long as the breakage also gets reported to each upstream.

Please ask upstreams to actually use the namespace (that's what it's for!) instead of just adding the flag. Adding the flag is a bandage for the time being, but not the best practice.

@ilovezfs
Copy link
Contributor

Please ask upstreams to actually use the namespace (that's what it's for!) instead of just adding the flag. Adding the flag is a bandage for the time being, but not the best practice.

@srl295 Definitely. We will ask them to use the namespace. We will not ask them to add the flag to their build scripts.

@commitay commitay closed this in 9ca5fba Mar 29, 2018
@commitay commitay deleted the icu4c-61.1 branch March 29, 2018 07:06
@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants