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 59.1 #12461

Closed
wants to merge 27 commits into from
Closed

icu4c 59.1 #12461

wants to merge 27 commits into from

Conversation

neutric
Copy link
Contributor

@neutric neutric commented Apr 15, 2017

Created with brew bump-formula-pr.

@DomT4
Copy link
Member

DomT4 commented Apr 15, 2017

Heh, these are always fun. These formulae will need revision bumps:

beecrypt
couchdb
dwdiff
freeciv
freeling
harfbuzz
libcdr
libfreehand
liblcf
libmspub
libphonenumber
libvisio
mapnik
mapnik@2
mpd
node
pazpar2
planck
sile
szl
widelands
yaz
zebra
zorba

@neutric
Copy link
Contributor Author

neutric commented Apr 15, 2017

@DomT4 Thanks for compiling the list! I'm on it! One question though before I start: http://site.icu-project.org/download/59 says that icu4c now requires C++11 language features and libraries, but the compilation seems to have worked fine without changing option :cxx11 to needs :cxx11. Does that mean I should leave it that way? And why bump the revisions of all those formulae you listed above and not just the ones out of brew uses --recursive icu4c that have linkage problems or break otherwise? freeciv for instance seems fine.

@DomT4
Copy link
Member

DomT4 commented Apr 15, 2017

One question though before I start: http://site.icu-project.org/download/59 says that icu4c now requires C++11 language features and libraries, but the compilation seems to have worked fine without changing option :cxx11 to needs :cxx11

Probably because all even semi-recent Xcode/CLT releases ship a compiler that can handle C++11 stuff without issue.

icu4c calls plenty of clang++ during compile, so I would guess before now the rule upstream was if we can find C++11 support we'll use it, if not not ideal but press ahead. On that wording it seems like the rule is now if we can't find it bail out hard, so things will need changing to needs :cxx11 and also probably everything applicable like this tweaked.

And why bump the revisions of all those formulae you listed above and not just the ones out of brew uses --recursive icu4c that have linkage problems or break otherwise? freeciv for instance seems fine.

Generally every major icu4c release changes the libraries' version numbers, which smashes up the linkage, but I guess it more or less comes down to which parts of icu4c said formula is using & how.

The last major bump was in #8224; you can see which formulae got bumped that time, so maybe start with that list & see what else blows up in testing? Whilst bearing in mind of course that that list was before several major taps were imported into core IIRC.

@ilovezfs
Copy link
Contributor

@srl295 we have (at least) three choices here:

  1. re-vendor icu4c into the node formula
  2. attempt to backport the v8 side of the fixes
  3. block this PR on the next node release

Any thoughts?

Copy link
Contributor

@ilovezfs ilovezfs left a comment

Choose a reason for hiding this comment

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

We'll need to sort out the node situation with upstream before proceeding.

@DomT4
Copy link
Member

DomT4 commented Apr 17, 2017

Ref on the Node side: nodejs/node#12077

@ilovezfs
Copy link
Contributor

indeed

@DomT4
Copy link
Member

DomT4 commented Apr 17, 2017

I'd vendor personally given icu4c doesn't actually need building in the node formula IIRC, you just feed it the location & it handles the rest, so the amount of additional code required isn't that significant. You da boss though 😸

@ilovezfs
Copy link
Contributor

@DomT4 honestly I'd prefer to wait on node to get this updated than to vendor icu4c forever.

@srl295
Copy link
Contributor

srl295 commented Apr 18, 2017

Node is doing a a v8 bump that will obviate needing a floating patch to v8. Plan to open 59.1 PR on node soon. Thx for the mention.

@ilovezfs
Copy link
Contributor

@srl295 You're welcome. Thanks for the status update!

@srl295
Copy link
Contributor

srl295 commented Apr 18, 2017

nodejs/node#12486 opened… but, blocked on the v8 issue.

@ilovezfs
Copy link
Contributor

ilovezfs commented Apr 19, 2017

@neutric It looks like we have non-node issues with

gjs patch:

diff --git a/Formula/gjs.rb b/Formula/gjs.rb
index 46a33f8..1cb3a17 100644
--- a/Formula/gjs.rb
+++ b/Formula/gjs.rb
@@ -27,6 +27,8 @@ class Gjs < Formula
   end
 
   def install
+    ENV.cxx11
+    ENV["_MACOSX_DEPLOYMENT_TARGET"] = ENV["MACOSX_DEPLOYMENT_TARGET"]
     resource("mozjs38").stage do
       inreplace "config/rules.mk", "-install_name @executable_path/$(SHARED_LIBRARY) ", "-install_name #{lib}/$(SHARED_LIBRARY) "
       cd("js/src") do
@@ -60,7 +62,6 @@ class Gjs < Formula
       # remove mozjs static lib
       rm "#{lib}/libjs_static.ajs"
     end
-    ENV.cxx11
     system "./configure", "--disable-debug",
                           "--disable-dependency-tracking",
                           "--disable-silent-rules",

szl patch

diff --git a/src/engine/form.cc b/src/engine/form.cc
index 9a64baf..5cef0e7 100644
--- a/src/engine/form.cc
+++ b/src/engine/form.cc
@@ -802,7 +802,7 @@ bool ArrayForm::IsEqual(Val* v1, Val* v2) const {
 Val* ArrayForm::Cmp(Val* v1, Val* v2) const {
   assert(v1->is_array());
   if (!v2->is_array())
-    return false;
+    return 0;
   ArrayVal* a1 = v1->as_array();
   ArrayVal* a2 = v2->as_array();
   assert(a1->type()->IsEqual(a2->type(), false));  // ignore proto info
@@ -1076,7 +1076,7 @@ bool TupleForm::IsEqual(Val* v1, Val* v2) const {
 Val* TupleForm::Cmp(Val* v1, Val* v2) const {
   assert(v1->is_tuple());
   if (!v2->is_tuple())
-    return false;
+    return 0;
   TupleVal* t1 = v1->as_tuple();
   TupleVal* t2 = v2->as_tuple();
   assert(t1->type()->IsEqual(t2->type(), false));  // ignore proto info

@srl295
Copy link
Contributor

srl295 commented Apr 19, 2017 via email

@srl295
Copy link
Contributor

srl295 commented May 9, 2017

@ilovezfs 59.1 just landed in node

@stale stale bot added the stale No recent activity label Jun 1, 2017
@stale
Copy link

stale bot commented Jun 1, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@JCount JCount added help wanted Task(s) needing PRs from the community or maintainers in progress Stale bot should stay away labels Jun 2, 2017
@stale stale bot removed the stale No recent activity label Jun 2, 2017
@srl295
Copy link
Contributor

srl295 commented Jul 24, 2017

@JCount what help is needed?

We'll need to sort out the node situation with upstream before proceeding.

That's been sorted as noted above.

@JCount
Copy link
Contributor

JCount commented Jul 24, 2017

@srl295 I added the "help wanted" label because neutric has been a bit busy, so someone else may want to pick this up.

@ilovezfs ilovezfs closed this Aug 12, 2017
@ilovezfs ilovezfs removed help wanted Task(s) needing PRs from the community or maintainers in progress Stale bot should stay away labels Aug 12, 2017
@ilovezfs
Copy link
Contributor

Picking this up in #16720.

@ilovezfs
Copy link
Contributor

@srl295 59 has shipped :)

@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.

None yet

5 participants