Bug 222225 - emulators/open-vm-tools: fails to set C++11 or later as required by ICU >= 59 on FreeBSD 10.3
Summary: emulators/open-vm-tools: fails to set C++11 or later as required by ICU >= 59...
Status: Closed Overcome By Events
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Josh Paetzel
URL:
Keywords: patch
Depends on:
Blocks: 222433
  Show dependency treegraph
 
Reported: 2017-09-11 13:07 UTC by Jan Beich
Modified: 2018-02-06 15:46 UTC (History)
2 users (show)

See Also:
bugzilla: maintainer-feedback? (jpaetzel)


Attachments
Respect language used during build (1.17 KB, patch)
2017-09-20 03:07 UTC, Jan Beich
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jan Beich freebsd_committer 2017-09-11 13:07:20 UTC
stringxx/string.cc:927:23: error: use of undeclared identifier 'Unicode_ToTitle'
   return CopyAndFree(Unicode_ToTitle(c_str(), locale), free);
                      ^

http://package23.nyi.freebsd.org/data/103i386-default-PR218788/2017-05-17_09h18m57s/logs/errors/open-vm-tools-10.1.0_2,2.log
Comment 1 Josh Paetzel freebsd_committer 2017-09-11 15:55:15 UTC
ICU 59 has a bunch of breaking changes.  Will the ports tree have both versions in it for a while?
Comment 2 Jan Beich freebsd_committer 2017-09-11 16:10:38 UTC
No as that'd complicate maintenance. If one of this port dependencies dependes on a different ICU version expect crashes. Upstream doesn't provide ABI compatibility beyond minor versions and downstream disabled symbol renaming (per bug 205120 comment 12).
Comment 3 Jan Beich freebsd_committer 2017-09-11 16:27:47 UTC
Does the port pass -std=c++11 when compiling sample code during configure stage? Builds fine on

11.0 amd64: http://sprunge.us/jbYE
11.0 i386:  http://sprunge.us/CQKi
Comment 4 Josh Paetzel freebsd_committer 2017-09-11 16:38:21 UTC
I'm a tad confused by your responses.

You are working on upgrading ICU to 59 correct?

The port builds fine against 58 but not 59.  So you want a patch to this port that can be committed when the ICU version changes get committed because maintaining two versions of ICU isn't what you want to do?

I don't understand the C++11 comment at all.
Comment 5 Jan Beich freebsd_committer 2017-09-11 17:12:21 UTC
The only difference in defines between 10.3 and 11.0 is -DHAVE_ICU_38. It's based on compiling sample ICU code during configure which fails (see below). Upstream needs to add a similar check to libsigc++-2.0 >= 2.5.1 which also requires -std=c++11. For now, I'll land a workaround under portmgr "just fix it" blanket.

In file included from conftest.cpp:28:
In file included from /usr/local/include/unicode/ucasemap.h:24:
In file included from /usr/local/include/unicode/utypes.h:38:
/usr/local/include/unicode/umachine.h:347:13: error: unknown type name 'char16_t'
    typedef char16_t UChar;
            ^
1 error generated.

(In reply to Josh Paetzel from comment #4)
> You are working on upgrading ICU to 59 correct?

Yep, but leaf consumers cannot block it unless there're a lot. My goal is before 2017Q4 branches (on 2017-10-01), otherwise it'd complicate maintenance of the branch itself and some consumers (e.g., www/firefox). I'm asking maintainers to help, if they don't such ports can be marked BROKEN under maintainer timeout. Obviously, that wouldn't be the end as fixes for the remaining bustage can be easily backported to 2017Q4 under ports-secteam blanket.
Comment 6 commit-hook freebsd_committer 2017-09-11 17:17:55 UTC
A commit references this bug:

Author: jbeich
Date: Mon Sep 11 17:17:20 UTC 2017
New revision: 449661
URL: https://svnweb.freebsd.org/changeset/ports/449661

Log:
  emulators/open-vm-tools: unbreak with ICU >= 59

  stringxx/string.cc:927:23: error: use of undeclared identifier 'Unicode_ToTitle'
     return CopyAndFree(Unicode_ToTitle(c_str(), locale), free);
                        ^

  PR:		222225

Changes:
  head/emulators/open-vm-tools/Makefile
Comment 7 Josh Paetzel freebsd_committer 2017-09-11 19:03:44 UTC
Does this commit just chsnge the compiler to GCC?

If so I'm not sure this is the correct way to go about this.
Comment 8 Jan Beich freebsd_committer 2017-09-11 20:09:30 UTC
No. GCC is unsafe to use in ports that have C++ dependencies, see bug 221288.

USE_CXXSTD=gnu++11 is converted to CXXFLAGS+=-std=gnu++11. Why -std=gnu++* rather than -std=c++*? Because both GCC and Clang default to -std=gnu++14 or -std=gnu++98 when unspecified. The port builds fine with ICU 59.1 using either -std=gnu++11 or -std=c++11. Ideally, -std=gnu++14 would be preferred to match default GCC version but libc++ in /stable/10 base doesn't support C++14.

USES=compiler:c++11-lib simply advertises the port needs C++11 capable compiler *and* library. On i386 and amd64 it's a nop but on some Tier2 architectures like mips*, powerpc*, sparc64 it'd switch from base GCC 4.2.1 (patched) to modern GCC from ports. However, it's not like -std=c++11 as required by libsigc++ >= 2.5.1 was safe to use with an ancient compiler.
Comment 9 Jan Beich freebsd_committer 2017-09-11 20:37:19 UTC
Do you want the commit backed out?
Comment 10 John Wolfe 2017-09-11 20:39:24 UTC
(In reply to Jan Beich from comment #9)

Let the change stand and we will work forward from there.
Comment 11 commit-hook freebsd_committer 2017-09-12 13:02:31 UTC
A commit references this bug:

Author: jbeich
Date: Tue Sep 12 13:01:24 UTC 2017
New revision: 449685
URL: https://svnweb.freebsd.org/changeset/ports/449685

Log:
  Back out C++11 changes for ICU >= 59

  r449608, r449609, r449610, r449611, r449612, r449613, r449614,
  r449621, r449661, r449662 are reverted. ICU will pull char16_t typedef
  in C++98 mode instead.

  Pointy hat to:	jbeich
  PR:		218788 222222 222225

Changes:
  head/databases/evolution-data-server/Makefile
  head/devel/icu-le-hb/Makefile
  head/devel/pecl-intl/Makefile
  head/devel/php70-intl/Makefile
  head/devel/php71-intl/Makefile
  head/devel/py-pyicu/Makefile
  head/emulators/open-vm-tools/Makefile
  head/lang/phantomjs/Makefile
  head/misc/sword/Makefile
  head/print/texlive-base/Makefile
  head/www/cppcms/Makefile
Comment 12 Jan Beich freebsd_committer 2017-09-12 14:54:29 UTC
The actual fix alluded in the commit message: https://reviews.freebsd.org/differential/changeset/?ref=316632
It seems AIX is better supported upstream than FreeBSD. ;\
Comment 13 Jan Beich freebsd_committer 2017-09-18 21:40:34 UTC
Reviving the issue. devel/icu workaround is PITA to maintain.
Comment 14 Jan Beich freebsd_committer 2017-09-18 22:31:25 UTC
On FreeBSD 10.3 libc++ doesn't define char16_t via <__config> which fails -DHAVE_ICU_38 configure check thus breaking build later.

$ cat >conftest.cpp
#include <unicode/ucasemap.h>

int main(void)
{
  (void) &ucasemap_utf8ToTitle;
  return 0;
}

$ clang++ $(icu-config --cppflags --ldflags) conftest.cpp
In file included from conftest.cpp:1:
In file included from /usr/local/include/unicode/ucasemap.h:24:
In file included from /usr/local/include/unicode/utypes.h:38:
/usr/local/include/unicode/umachine.h:347:13: error: unknown type name 'char16_t'
    typedef char16_t UChar;
            ^
1 error generated.
Comment 15 Jan Beich freebsd_committer 2017-09-18 22:50:34 UTC
<__config> is bootlegged via <stddef.h> is due to https://llvm.org/viewvc/llvm-project?view=revision&revision=249761
Comment 16 John Wolfe 2017-09-19 04:30:20 UTC
Can we get access to some preliminary ICU 59.1 packages for 10.3 and 11.1 for the i386 and amd64 platforms?

We want to help, but to do so, we need to have something to build against.  

Thanks
Comment 17 Jan Beich freebsd_committer 2017-09-19 19:28:20 UTC
Sorry, I can't publish packages built on my personal machine(s). The build artifacts are just a cache from testing multitude of patches (mine and not) at the same time before landing. Often some packages are out of sync with each other as I only build individual ports, not the whole tree. For anything usable or trustworthy ask a different person or set up a FreeBSD box, apply the patches from bug 218788 + bug 222433 and build yourself. ICU itself doesn't have a lot of dependencies.
Comment 18 Josh Paetzel freebsd_committer 2017-09-19 20:31:31 UTC
I'll take care of this and build images with ICU 59 and push the repo public.  Are you done with the current repo?
Comment 19 Jan Beich freebsd_committer 2017-09-19 21:13:02 UTC
(In reply to Josh Paetzel from comment #18)
> Are you done with the current repo?

Do you bug 218788 changes? Yep, barring review from maintainer. OTOH, bug 222433 is an attempt to prepare for ICU 60.1 in advance (release scheduled on 2017-10-18). Whether C++11 requirement would be blocking is still undecided.
Comment 20 John Wolfe 2017-09-19 21:53:03 UTC
(In reply to Josh Paetzel from comment #18)
> Are you done with the current repo?

Yes, I will let QE know how to access the 10.1.10 packages from head should they need them in the meantime.

It is understood that if ICU 59.1 lands, open-vm-tools-10.1.10 may not build until we get this addressed.
Comment 21 Josh Paetzel freebsd_committer 2017-09-19 22:46:04 UTC
John:

That is incorrect.  The ICU 59 port was fixed so open-vm-tools builds against it.

Whether it works or not is a different story...
Comment 22 John Wolfe 2017-09-19 23:22:39 UTC
(In reply to Josh Paetzel from comment #21)
This issue was re-opened in comment 13.  

My assumption is that if an issue has been opened for a compilation/configuration issue, at least one of the target builds will fail.
Comment 23 Josh Paetzel freebsd_committer 2017-09-19 23:57:55 UTC
The problem that was causing the compilation failure against ICU 59 was addressed in ICU.

Bug closed.
Comment 24 Jan Beich freebsd_committer 2017-09-20 00:14:27 UTC
(In reply to Josh Paetzel from comment #23)
Do you plan to ignore bustage related to bug 222433?
Comment 25 Josh Paetzel freebsd_committer 2017-09-20 01:55:10 UTC
I have no idea what you are talking about.  What is exactly the issue with the bug you are referencing?
Comment 26 Jan Beich freebsd_committer 2017-09-20 02:21:07 UTC
After both bug 218788 + bug 222433 are applied this port fails to build on FreeBSD 10.3 as described in comment 0. Bug 218788 will land very soon, bug 222433 sometime after. Once ICU 60.* approaches a release candidate bug 222433 landing timeline should become more clear.
Comment 27 Jan Beich freebsd_committer 2017-09-20 03:07:59 UTC
Created attachment 186559 [details]
Respect language used during build

Maybe a patch can help you understend what configure does wrong. When building C++ code against ICU it should consult with "icu-config --cxxflags" (i.e., via m4/vmtools.m4) but all files with #include <unicode/*> are C code.
Comment 28 Jan Beich freebsd_committer 2017-09-21 17:29:23 UTC
Pending review on the proposed patch.
Comment 29 Josh Paetzel freebsd_committer 2017-09-25 22:10:29 UTC
Jan,

What we are testing now is using open-vm-tool's internal ICU implementation and not using an external icu dep at all.

Stay tuned.
Comment 30 Jan Beich freebsd_committer 2017-09-25 22:59:28 UTC
ICU upstream doesn't make new patch-level releases to fix vulnerabilities. I just hope you're aware of
https://www.freebsd.org/doc/en/books/porters-handbook/bundled-libs.html
https://security-tracker.debian.org/tracker/source-package/icu # many not in VuXML
Comment 31 John Wolfe 2017-09-25 23:43:37 UTC
(In reply to Josh Paetzel from comment #29)
(In reply to Jan Beich from comment #30)

Open-vm-tools is not bundling an internal copy of the ICU libraries in its release.   I apologize to Josh for stating the details in such a way that suggested that may be the case.

Open-vm-tools uses a limited bit of unicode in it UI.  As you know from trying to compile the open-vm-tools with ICU 59.1, VMware's string library makes use of the function Unicode_ToTitle() function which is no longer provided by ICU 59.1.  As it turns out we already have a C implementation of that function in the VMware provided common runtime.

The next major release of vmtools will not have a dependency on the open-source ICU packages.  The underlying changes are already in the VMware libraries provided with open-vm-tools 10.1.10 and later.  Making the switch did not meet the severity threshold to be included in a 10.1.x point release.

With the impending upgrade to ICU 59.1 on FreeBSD for 2017Q4, the time to make that configuration switch is now.
Comment 32 Jan Beich freebsd_committer 2017-09-26 00:07:54 UTC
(In reply to John Wolfe from comment #31)
> VMware's string library makes use of the function Unicode_ToTitle()
> function which is no longer provided by ICU 59.1.

Unicode_ToTitle() is a private function of open-vm-tools. It was never part of any ICU release. If you mean ucasemap_utf8ToTitle() then it wasn't changed at all in ICU 59.1.

https://abi-laboratory.pro/tracker/compat_report/icu4c/58_2/59_1/eb08d/abi_compat_report.html

> With the impending upgrade to ICU 59.1 on FreeBSD for 2017Q4,
> the time to make that configuration switch is now.

ICU 59.1 has landed yesterday but the package cluster hasn't caught up yet. The proposed patch here has zero impact on the produced binaries.
Comment 33 Jan Beich freebsd_committer 2018-02-06 15:46:16 UTC
Obsoleted by ports r451100.