Bug 265180 - www/chromium: actually use the system-installed ICU
Summary: www/chromium: actually use the system-installed ICU
Status: Open
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Some People
Assignee: freebsd-chromium (Nobody)
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2022-07-12 21:43 UTC by Mikhail T.
Modified: 2022-07-20 18:08 UTC (History)
3 users (show)

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


Attachments
Use the already-installed ICU (2.08 KB, patch)
2022-07-12 21:43 UTC, Mikhail T.
no flags Details | Diff
0001-www-chromium-unbundle-icu-to-use-icu-from-ports.patch (1.85 KB, patch)
2022-07-16 10:18 UTC, Robert Nagy
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mikhail T. 2022-07-12 21:43:34 UTC
Created attachment 235226 [details]
Use the already-installed ICU

Though the port lists devel/icu among the LIB_DEPENDS, the upstream code builds the bundled version of ICU anyway...

This is true about a number of other bundled components too (AOM, AVIF, jsoncpp, node, v8, etc.), but ICU seems to be the easiest to fix.

The proposed patch adds ICU to the existing (incomplete) list of possible dependencies, and, additionally, ensures, the icu/source folder is never even extracted from the tarball. For good measure...

The post-patch gymnastics are only necessary because of the is_official_build=true setting -- do we really need it?
Comment 1 Robert Nagy 2022-07-13 05:17:27 UTC
(In reply to Mikhail T. from comment #0)

Due to the fact that built-in ICU has several modifications, I would
rather stick to it and remove the dependency from the port.

The same goes for all the other deps that are picked up internally instead
because there have been several incompatibility issues previously using
an external library.
Comment 2 Mikhail T. 2022-07-13 18:50:20 UTC
(In reply to Robert Nagy from comment #1)

> built-in ICU has several modifications

If these modifications are good and useful, Google can/should submit them to IBM for inclusion into ICU releases. If IBM are too slow to process the contribution, FreeBSD can incorporate the changes ourselves -- they can be added (as a patch) to our devel/icu -- to benefit ALL of ICU-using software. Plenty of precedent there.

To have ports use their own versions -- of ANYTHING -- is not right (see link above). The actual work on unbundling may be substantial -- which would explain, why it hasn't happened yet. But that such unbundling is a goal, is beyond dispute.

Chromium port is not the first to stumble into this -- OpenOffice.org, for example, was quite notorious too some years ago.

> The same goes for all the other deps that are picked up internally

This view is held by a number of people, but FreeBSD Ports have a clearly-stated policy to the contrary:

https://docs.freebsd.org/doc/10.1-RELEASE/usr/local/share/doc/freebsd/en/books/porters-handbook/bundled-libs.html

Ideally, we'll get to the point, where the entire third_party/ subdirectory is not even extracted from the upstream's tar-ball.

> there have been several incompatibility issues previously

Frankly, this is FUD :-)
Comment 3 Graham Perrin freebsd_committer freebsd_triage 2022-07-13 18:56:05 UTC
(In reply to Mikhail T. from comment #2)

> … <https://docs.freebsd.org/doc/10.1-RELEASE/usr/local/share/doc/freebsd/en/books/porters-handbook/bundled-libs.html> …

In the current edition of the FreeBSD Porter's Handbook: 

<https://docs.freebsd.org/en/books/porters-handbook/book/#bundled-libs>
Comment 4 Robert Nagy 2022-07-16 10:18:34 UTC
Created attachment 235289 [details]
0001-www-chromium-unbundle-icu-to-use-icu-from-ports.patch

I am not against doing this but let's keep it as simple as possible.
Please test the attached patch.
Comment 5 Mikhail T. 2022-07-19 17:40:34 UTC
(In reply to Robert Nagy from comment #4)
This simple approach is, what I started with. However, this is dangerous -- because the bundled ICU header-files are used, instead of those installed by the icu-port.

This is why my proposal includes removal of the bundled ICU...
Comment 6 commit-hook freebsd_committer freebsd_triage 2022-07-19 19:58:16 UTC
A commit in branch 2022Q3 references this bug:

URL: https://cgit.FreeBSD.org/ports/commit/?id=b2193c3c801075a5b0d985d50b2e25dba4c9b6b4

commit b2193c3c801075a5b0d985d50b2e25dba4c9b6b4
Author:     Robert Nagy <robert@openbsd.org>
AuthorDate: 2022-07-16 10:15:36 +0000
Commit:     Rene Ladan <rene@FreeBSD.org>
CommitDate: 2022-07-19 19:56:43 +0000

    www/chromium: unbundle icu to use icu from ports

    (cherry picked from commit 55b20cdeb6569e0e3c63d58aedd679901d15dc50)

    PR:             265180

 www/chromium/Makefile | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
Comment 7 Mikhail T. 2022-07-20 18:08:06 UTC
The committed change is insufficient -- instead of using the headers installed by devel/icu, the port still uses the bundled ones.

A simple experiment:
 1. make patch
 2. Edit the ${WRKSRC}/third_party/icu/source/common/unicode/utypes.h to insert the line at the beginning:
   #error Meow-meow
 3. make
 4. Observe the build failing with:
../../third_party/icu/source/common/unicode/utypes.h:1:2: error: Meow-meow

As I mentioned in Comment #5, this -- taking declarations from one source and definitions from another -- is dangerous.

Please, consider using my patch -- or some derivative thereof -- instead.