Opened 11 years ago

Closed 11 years ago

#36495 closed defect (fixed)

icu @49.1.2_0 Patch to allow successful build on Tiger

Reported by: ccarey@… Owned by: macports-tickets@…
Priority: Normal Milestone:
Component: ports Version: 2.1.2
Keywords: tiger haspatch Cc: ryandesign (Ryan Carsten Schmidt), drkp (Dan Ports), ned-deily (Ned Deily), ballapete (Peter "Pete" Dyballa)
Port: icu

Description

The icu @49.1.2_0 port does not successfully build on Tiger because of

:info:build /usr/bin/g++-4.0 -DU_ATTRIBUTE_DEPRECATED= -DU_COMMON_IMPLEMENTATION    -DU_HAVE_TIMEZONE=0  -I.   "-DDEFAULT_ICU_PLUGINS=\"/opt/local/lib/icu\" " -pipe -O2 -arch ppc -O2 -W -Wall -ansi -pedantic -Wpointer-arith -Wwrite-strings -Wno-long-long   -fno-common -c   -o putil.ao putil.cpp
:info:build putil.cpp: In function ‘const char* uprv_tzname_49(int)’:
:info:build putil.cpp:1083: error: ‘localtime_r’ was not declared in this scope
:info:build putil.cpp: At global scope:
:info:build putil.cpp:1450: warning: ‘const char* age()’ defined but not useddepp
:info:build gnumake[1]: *** [putil.ao] Error 1

The attached patch fixes the problem by providing a “backup” declaration of localtime_r() for Darwin. Note that the platform-specific #defines within ICU only distinguish Darwin from other operating systems; they don’t distinguish between Tiger, Leopard, Snow Leopard, &c. This patch works for Tiger, but could theoretically cause a problem for other versions of Mac OS X. I don’t have access to any other Mac OS X version than Tiger, so please ensure that this patch causes no problem with Leopard and beyond before integrating this patch.

Attachments (2)

putil.cpp.diff (870 bytes) - added by ccarey@… 11 years ago.
patch to source/common/putil.cpp to allow icu @49.1.2_0 to build successfully on Tiger
putil.cpp.2.diff (884 bytes) - added by ultrajoe@… 11 years ago.
"Cleaned up" version useful for average user usage

Download all attachments as: .zip

Change History (18)

Changed 11 years ago by ccarey@…

Attachment: putil.cpp.diff added

patch to source/common/putil.cpp to allow icu @49.1.2_0 to build successfully on Tiger

comment:1 in reply to:  description ; Changed 11 years ago by ryandesign (Ryan Carsten Schmidt)

Replying to ccarey@…:

The icu @49.1.2_0 port does not successfully build on Tiger because of

Have you already reported this problem to the developers of icu? If so please post a URL to the bug report or mailing list post, if possible.

comment:2 Changed 11 years ago by ryandesign (Ryan Carsten Schmidt)

Cc: ryandesign@… added

Cc Me!

comment:3 Changed 11 years ago by ultrajoe@…

For the record, this also holds true building with apple-gcc-4.2.

I'll try the patch.

comment:4 in reply to:  1 ; Changed 11 years ago by ccarey@…

Replying to ryandesign@…:

Have you already reported this problem to the developers of icu?

No, I haven’t reported it to them yet. I thought that if this patch didn’t cause any problems with Leopard and beyond within MacPorts, that the ICU developers would accept it more readily.

Changed 11 years ago by ultrajoe@…

Attachment: putil.cpp.2.diff added

"Cleaned up" version useful for average user usage

comment:5 Changed 11 years ago by drkp (Dan Ports)

Cc: dports@… added

This sounds like the same issue as #35452 (for texlive-bin's version of icu). I wasn't able to make any progress on that one for lack of a Tiger machine.

comment:6 in reply to:  5 Changed 11 years ago by ccarey@…

Replying to dports@…:

This sounds like the same issue as #35452 (for texlive-bin's version of icu).

Yes, it is the same issue. I’ve added a texlive-bin-specific patch to #35452.

comment:7 Changed 11 years ago by ned-deily (Ned Deily)

Cc: nad@… added

Cc Me!

comment:8 Changed 11 years ago by ballapete (Peter "Pete" Dyballa)

Six weeks after the patch to fix this version of icu I still have:

pete 294 /\ l -R /opt/local/var/macports/sources/lil.fr.rsync.macports.org/release/tarballs/ports/devel/icu
/opt/local/var/macports/sources/lil.fr.rsync.macports.org/release/tarballs/ports/devel/icu:
insgesamt 8
-rw-r--r-- 1 root wheel 7720  3. Okt 04:00 Portfile
drwxr-xr-x 5 root wheel  170  3. Okt 04:00 files

/opt/local/var/macports/sources/lil.fr.rsync.macports.org/release/tarballs/ports/devel/icu/files:
insgesamt 12
-rw-r--r-- 1 root wheel  739 21. Sep 22:23 patch-config-mh-darwin.diff
-rw-r--r-- 1 root wheel 1524  3. Okt 04:00 patch-configure.diff
-rw-r--r-- 1 root wheel 1663  3. Okt 04:00 patch-universal.diff

Do I have to manually apply that patch?

comment:9 Changed 11 years ago by ballapete (Peter "Pete" Dyballa)

Cc: Peter_Dyballa@… added

Cc Me!

comment:10 Changed 11 years ago by ballapete (Peter "Pete" Dyballa)

Manually applying the patch file putil.cpp.2.diff​ allowed to build icu.

comment:11 in reply to:  8 Changed 11 years ago by ccarey@…

Replying to Peter_Dyballa@…:

Do I have to manually apply that patch?

If you’re compiling on Tiger, then yes, at this point you still have to manually apply this patch.

As noted in this ticket’s description, this patch has only been tried on Tiger — at least by me, since it is the only Mac system to which I have access — but the nature of the patch means that it would be applied to all Mac OS X releases, not just Tiger. If this patch allows successful compilation on releases other than Tiger, then it would be OK to use with those releases. [On those other releases, this patch only adds an extra declaration for the localtime_r () function, so their runtime behavior wouldn’t be affected if the patched source compiles successfully]. Once this patch has been demonstrated to compile successfully against Leopard and beyond, then it could be integrated into icu.

comment:12 Changed 11 years ago by ballapete (Peter "Pete" Dyballa)

On PPC Leopard (10.5.8) and on intel Snow Leopard (10.6.8) the putil.cpp.2.diff​ patch was not needed to build icu @49.1.2. On these two platforms it built instantly.

comment:13 in reply to:  4 ; Changed 11 years ago by ryandesign (Ryan Carsten Schmidt)

Replying to ccarey@…:

Replying to ryandesign@…:

Have you already reported this problem to the developers of icu?

No, I haven’t reported it to them yet. I thought that if this patch didn’t cause any problems with Leopard and beyond within MacPorts, that the ICU developers would accept it more readily.

When I'm looking for patches to commit, I usually see it the other way around: I'm more likely to commit a patch if the developers have already signed off on it. I did apply it locally and verify that it still builds on Mountain Lion, but since I'm not a C programmer and I'm not familiar with the icu source code, I don't feel totally confident committing these changes. Could you report the problem and your patch to the developers so that we can get their input?

comment:14 in reply to:  12 Changed 11 years ago by ccarey@…

Replying to Peter_Dyballa@…:

On PPC Leopard (10.5.8) and on intel Snow Leopard (10.6.8) the putil.cpp.2.diff​ patch was not needed to build icu @49.1.2. On these two platforms it built instantly.

Yes, but the point is that this patch affects compilation on Leopard, Snow Leopard, Lion, and Mountain Lion as well as compilation on Tiger. Ideally, this patch should have absolutely no effect on compilation on Leopard and beyond, but since I only have access to a Tiger system, I can’t guarantee that that will be the case — which is why I’d asked for the patch to also be compiled on those systems, to ensure that this patch would have no effect on them, before integration of this patch into icu.

Version 0, edited 11 years ago by ccarey@… (next)

comment:15 in reply to:  13 Changed 11 years ago by ccarey@…

Replying to ryandesign@…:

Replying to ccarey@…:

Replying to ryandesign@…:

Have you already reported this problem to the developers of icu?

No, I haven’t reported it to them yet. I thought that if this patch didn’t cause any problems with Leopard and beyond within MacPorts, that the ICU developers would accept it more readily.

When I'm looking for patches to commit, I usually see it the other way around: I'm more likely to commit a patch if the developers have already signed off on it. I did apply it locally and verify that it still builds on Mountain Lion, but since I'm not a C programmer and I'm not familiar with the icu source code, I don't feel totally confident committing these changes. Could you report the problem and your patch to the developers so that we can get their input?

This problem had already been reported by someone else as ICU ticket 9367. I’ve added a comment to that ticket directing them here to this patch. Considering the number of outstanding tickets there (as well as the age of many of them), I don’t know what the likelihood might be of getting their input on this patch in a timely fashion.

comment:16 Changed 11 years ago by ryandesign (Ryan Carsten Schmidt)

Resolution: fixed
Status: newclosed

To avoid any surprises I've committed the patch in r100308 for Tiger only. Upstream can take it from here.

Note: See TracTickets for help on using tickets.