Bug 856594 - Disable ICU symbol renaming
Disable ICU symbol renaming
Status: CLOSED WONTFIX
Product: Fedora
Classification: Fedora
Component: icu (Show other bugs)
rawhide
All All
unspecified Severity high
: ---
: ---
Assigned To: Eike Rathke
Fedora Extras Quality Assurance
: Reopened
Depends On:
Blocks: 905069
  Show dependency treegraph
 
Reported: 2012-09-12 07:36 EDT by Thiago Macieira
Modified: 2013-01-29 06:18 EST (History)
10 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-01-29 04:56:13 EST
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
patch to define U_DISABLE_RENAMING by configure (23.72 KB, patch)
2013-01-26 22:00 EST, Mamoru TASAKA
no flags Details | Diff

  None (edit)
Description Thiago Macieira 2012-09-12 07:36:17 EDT
icu.spec builds ICU with the following configure line:

%configure --with-data-packaging=library --disable-samples

It's missing --disable-renaming. According to [1], when ICU is to be installed as a system library, symbol renaming should be disabled, so that binary compatibility can exist.

This bug does not affect each Fedora version per se, provided that Fedora never updates ICU from one minor release to another. (After 4.8, the minor number is now merged into the major, 49)

However, it affects third-party binaries and local compilations done on one machine. For example, an application compiled on Fedora 16 with ICU version 4.6 will not run on Fedora 17 without recompilation, even if said application was using only the symbols that ICU promises to be stable.


[1] http://userguide.icu-project.org/design#TOC-ICU-Binary-Compatibility:-Using-ICU-as-an-Operating-System-Level-Library
Comment 1 Caolan McNamara 2012-09-12 09:18:50 EDT
hmph, didn't even know icu had a --disable-renaming or I'd have done that ages ago, ah well :-(

Too late for F-17/F-18 to do this though, seeing as everything that links against icu there would have to be rebuilt. We can do that in rawhide however.
Comment 2 Thiago Macieira 2012-09-12 09:30:55 EDT
Can I beg for rebuilding F18? It's not released yet, so there's still time.
Comment 3 Caolan McNamara 2012-09-12 10:32:49 EDT
http://fedoraproject.org/wiki/Releases/18/Schedule looks a bit sucky in that respect for a late rebuild of anything using icu. You could bring it to the list and see what people think wrt the schedule.
Comment 4 Thiago Macieira 2012-09-12 11:25:05 EDT
Hmm... further investigation shows that --disable-renaming stops the renaming, but still keeps bumping the SONAME. This might be actually worse, so I'll leave it up to you to decide.

If the SONAME keeps bumping, compiled applications will stop working anyway. But without the renaming, it's impossible to have two copies of ICU loaded in memory because they might clash in unpredictable ways.
Comment 5 Yanko Kaneti 2012-09-12 11:44:40 EDT
That last icu build http://koji.fedoraproject.org/koji/buildinfo?buildID=353856 although keeping the soname seems to have broken rawhide thoroughly
Comment 6 Thiago Macieira 2012-09-12 11:58:53 EDT
It will require a full rebuild of everything.

Maybe it would be a good idea to apply --disable-renaming only when ICU decides to bump the SONAME again.

Or drop the idea until ICU devs figure out how to make libraries.
Comment 7 Yanko Kaneti 2012-09-12 12:15:57 EDT
FWIF harfbuzz which mostly breaks everything else wouldn't even build here with -6
Comment 8 Caolan McNamara 2012-09-12 16:04:54 EDT
hmm, yeah if the soname bumps on each release then there isn't much of an advantage. I'll revert (as icu-49.1.1-7.fc19) and flag the spec to have another look on the next icu release to see if it would be helpful at that stage.
Comment 9 Eike Rathke 2013-01-25 16:43:46 EST
Note: --disable-renaming again in icu-50.1.2-1.fc19
Comment 10 Kevin Fenzi 2013-01-26 15:24:32 EST
So, what should maintainers be doing here?

A number of packages (like say harfbuzz) fail to build against the new icu, and theres: 
# Add -DU_DISABLE_RENAMING=1 to compiler flags
# (ref: bug 856594)
export CXXFLAGS="%{optflags} $(icu-config --cppflags)"

now in the spec?
Is this something we expect many of the icu consuming packages to carry?
That seems suboptimal.
Comment 11 Eike Rathke 2013-01-26 17:40:10 EST
Argh.. does that mean that consumers of ICU have to use that flag as well? That would be a no-go.
Comment 12 Thiago Macieira 2013-01-26 18:54:24 EST
You can put that #define in one of the headers. Probably by patching it, since ICU's buildsystem does not offer the ability.
Comment 13 Mamoru TASAKA 2013-01-26 21:29:12 EST
(In reply to comment #12)
> You can put that #define in one of the headers. Probably by patching it,
> since ICU's buildsystem does not offer the ability.

Looks like this is more appropriate than patching pkgconfig file.
/usr/include/unicode/urename.h says:

/* U_DISABLE_RENAMING can be defined in the following ways:
 *   - when running configure, e.g.
 *        runConfigureICU Linux --disable-renaming
 *   - by changing the default setting of U_DISABLE_RENAMING in uconfig.h
 */

so, patching /usr/include/unicode/uconfig.h so that uconfig.h contains:
#define U_DISABLE_RENAMING 1
is perhaps appropriate.
Comment 14 Mamoru TASAKA 2013-01-26 22:00:15 EST
Created attachment 688250 [details]
patch to define U_DISABLE_RENAMING by configure

Or applying the attached patch will properly set the value of U_DISABLE_RENAMING with configure (with or without --disable-renaming).

The attached patch looks large, but this is because common/unicode/uconfig.h has to be renamed to common/unicode/uconfig.h.in. The diff of the original common/unicode/uconfig.h and new common/unicode/uconfig.h.in is:

$ diff -u icu/source/common/unicode/uconfig.h.{RENAME,in}
--- icu/source/common/unicode/uconfig.h.RENAME	2013-01-11 09:23:12.000000000 +0900
+++ icu/source/common/unicode/uconfig.h.in	2013-01-27 11:38:09.000000000 +0900
@@ -86,9 +86,7 @@
  * Determines whether to disable renaming or not.
  * @internal
  */
-#ifndef U_DISABLE_RENAMING
-#define U_DISABLE_RENAMING 0
-#endif
+#define U_DISABLE_RENAMING @U_DISABLE_RENAMING_VALUE@
 
 /**
  * \def U_NO_DEFAULT_INCLUDE_UTF_HEADERS
Comment 15 Mamoru TASAKA 2013-01-26 22:12:27 EST
With the attached patch, it should be that packages using icu need not adding "icu-config --cppflags".
Comment 16 Eike Rathke 2013-01-28 14:21:13 EST
Fearing a maintenance nightmare on updates with a renamed and modified uconfig.h file plus configure.in I dug around and saw uconfig.h.prepend being created by configure. Including that with the sed one-liner

sed -e '/^#define __UCONFIG_H__/ r icu/source/uconfig.h.prepend' -i icu/source/common/unicode/uconfig.h

should do the trick.

Thanks for pointing me in the right direction!
Comment 17 Thiago Macieira 2013-01-28 14:48:08 EST
Please make sure that the library soname does not change on an upgrade and that it's different from a versioned ICU library of the same version.
Comment 18 Eike Rathke 2013-01-28 15:00:36 EST
Sorry, but how should "[soname] different from a versioned ICU library of the same version" be accomplished?
Comment 19 Thiago Macieira 2013-01-28 15:46:10 EST
Well, if we have right now ICU 50 and the soname is libicui18n.so.50, and later upgrade to 51, we don't want the soname to change to libicui18n.so.51. That defeats the entire purpose of this bug report: everything would need to be recompiled.

In fact, it would make the situation worse: on a system where the old libs are still present for dependency reasons, both libs could be loaded at runtime but would clash. Right now they don't.

It seems to me that the soname must be chosen so that it's not tied to a specific version then.

My specific problem is that we deploy a pre-built binary that links to ICU. I must make sure that the binary we create has ICU libs with either a different soname or has a standard configuration (from upstream).
Comment 20 Eike Rathke 2013-01-28 16:34:55 EST
> we deploy a pre-built binary that links to ICU

Who is "we" and why is it a pre-built binary and what does it do in Fedora?
Comment 21 Thiago Macieira 2013-01-28 16:46:09 EST
(In reply to comment #20)
> > we deploy a pre-built binary that links to ICU
> 
> Who is "we" and why is it a pre-built binary and what does it do in Fedora?

It shouldn't matter. It's just a third-party binary that some Fedora users might wish to download, even if it is available on Fedora. Unfortunately, LSB is too ancient for our purposes.

In specific, the binary builds found in http://qt-project.org/downloads/
Comment 22 Eike Rathke 2013-01-28 17:06:30 EST
Well, are those binaries built using the ICU guidelines? i.e.

1. Applications must use only APIs that are marked as stable.
2. Applications must use only plain C APIs, never C++.
4. Applications must be built using an ICU that was configured for binary compatibility.
Comment 23 Thiago Macieira 2013-01-28 17:16:58 EST
(In reply to comment #22)
> Well, are those binaries built using the ICU guidelines? i.e.
> 
> 1. Applications must use only APIs that are marked as stable.

Yes

> 2. Applications must use only plain C APIs, never C++.

Yes

> 4. Applications must be built using an ICU that was configured for binary
> compatibility.

Heh, that's a weird requirement considering they don't offer binary compatibility... we build using their script, without using the library suffix options.

Anyway, the point is that we build like Linux distributions do (including Fedora 18). If the library with the same soname is present on the system, we currently have the same C API, including the renaming.

That's why I'm asking that the soname be different if Fedora stops the symbol renaming.
Comment 24 Kevin Fenzi 2013-01-28 17:25:24 EST
I'm not an ICU maintainer, but what you are asking for sounds like something to discuss with upstream if I am understanding you correctly. (ie, you want a soname to stay the same if it's binary compatible). 

I don't think it's Fedora's job to carry some incompatible patch for that, you should ask upstream about this... 

all IMHO.
Comment 25 Eike Rathke 2013-01-28 17:55:54 EST
(In reply to comment #23)
> > 4. Applications must be built using an ICU that was configured for binary
> > compatibility.
> 
> Heh, that's a weird requirement considering they don't offer binary
> compatibility...

I just cited verbatim from the ICU doc's section you gave the URL for in your original description.

> That's why I'm asking that the soname be different if Fedora stops the
> symbol renaming.

I could be enlightened if you told me what exactly to do, i.e. how to configure and build ICU for this specific case. This mess needs to be solved ASAP as F19 is currently in a state that maintainers hold back their package builds until this is solved. If there's no real solution right now then we'll have to either live on with disabled renaming but soname still bumping until sorted out and having the same soname for unrenamed as renamed build, or roll back the change and build without --disable-renaming again.
Comment 26 Thiago Macieira 2013-01-28 18:25:01 EST
I'd say that we should go back to what it was in F18 (drop --disable-renaming). ICU maintainers don't know how to make library releases. The current situation is sub-optimal but acceptable.

Meanwhile, I will approach them about making proper library releases.
Comment 27 Eike Rathke 2013-01-29 04:56:13 EST
icu-50.1.2-3.fc19 goes back to where we have been before. I promise I will not touch this again..
Comment 28 Kalev Lember 2013-01-29 05:57:38 EST
OK, but have you actually verified that symbols in the new icu rpm now have the _50 prefix again? I am asking because I still see a sed command in the spec file that edits the headers and seems to be related to the symbol renaming.

Also, note that if you rename the symbols, this is an ABI change and all the icu consumers will need one more rebuild. And RPM won't be able to tell us what exactly needs a rebuild, because the library soname has not changed this time.

Can you post a heads up to the -devel list that there's one more full icu rebuild needed?
Comment 29 Eike Rathke 2013-01-29 06:12:36 EST
(In reply to comment #28)
> OK, but have you actually verified that symbols in the new icu rpm now have
> the _50 prefix again?

Suffix.. yes, they do.

> I am asking because I still see a sed command in the
> spec file that edits the headers and seems to be related to the symbol
> renaming.

Actually that command inserts the content of a configure generated uconfig.h.prepend, I left it in in case we'd use other options that do so in future, but currently no uconfig.h.prepend is generated, which is why I added the   test -f uconfig.h.prepend

> Also, note that if you rename the symbols, this is an ABI change and all the
> icu consumers will need one more rebuild. And RPM won't be able to tell us
> what exactly needs a rebuild, because the library soname has not changed
> this time.
> 
> Can you post a heads up to the -devel list that there's one more full icu
> rebuild needed?

Already did that, see mail with subject "icu 50 rebuilt without --disable-renaming again"
Comment 30 Kalev Lember 2013-01-29 06:18:54 EST
Sounds good, thanks. We'll also have a F19 mass rebuild coming up on Friday, so if there are some icu consumers that haven't been rebuilt by that time, they'll hopefully get fixed with the mass rebuild.

Note You need to log in before you can comment on or make changes to this bug.