Bug#928111: [pre-approval] unblock: icu/63.2-1
Hi Paul and Ivo, On Tue, Jun 18, 2019 at 7:23 PM Paul Gevers wrote: > On 17-06-2019 22:05, László Böszörményi (GCS) wrote: > > On Mon, Jun 17, 2019 at 9:50 PM Paul Gevers wrote: > > >> Do I understand correctly that what we are now getting with this version > >> is: > >> > >> - Reiwa support > >> - fix for the speed regression? This is a friendly ping on how you decide on ICU. As a recap, ICU 63 is in a maintenance mode with only bugfixes applied. As of this, the ICU 63.2-2 in Sid which should be part of Buster contains the following changes over 63.1-6: - speed up one of its working (MutableCodePointTrie.build()) with hashtables, - include the previously backported security fixes, - adds Japanese Reiwa support, - updates to the IANA tzdata 2019a release. It's in our archives _and_ in Ubuntu for almost three weeks without any regression. Thanks for consideration, Laszlo/GCS
Bug#928111: [pre-approval] unblock: icu/63.2-1
Control: tags -1 -moreinfo Hi Paul, On Tue, Jun 18, 2019 at 7:23 PM Paul Gevers wrote: > There are quite a lot of changes in the diff, and only at the end we get > to the Reiwa support (if I understand the code and comments at all). You > haven't explained these other changes. If all we are now talking about > is Reiwa support I will *not* unblock the current version of icu, as it > introduces changes that are not targeted at fixing that. Indeed, there are other changes. Let me go into more details via the upstream commit list[1] which is on the maint-63 branch (important bugfixes only for the 63 release). The 63.1 release happened on Oct 13, 2018 with the last commit of ICU-20205 (RelativeDateTimeFormatter pt data fix, improved error handing and test)[2]. The first fix is ICU-20208 (uspoof.cpp function checkImpl should be static) [3] and I've backported it with ICU-20246 (Fixing another integer overflow in number parsing) [4] (security issues) to (and already in) Buster. Fixes between these commits (ICU-20214 Fix namespace error on Cygwin, ICU-20209 Fix build failures on Windows and ICU-20240 Fix Windows build failure) [5] are for Windows only and don't affect us. Next is a faster operation change with hashtables (ICU-20250 faster MutableCodePointTrie.build()) [6]. An additional speed change is the startup regression change (ICU-20250 make UnicodeSet(intprop=value) faster) [7] that I had to reverse apply (meaning not part of the source in this unblock request) due to its ABI break. The next commit is for the Java ICU source [8] (ICU is split in Debian to src:icu and src:icu4j), ICU-20255 revert to reflection for methods not yet in Android API thus not part of this unblock request. Then comes the Japanese era Reiwa support commits [9] (ICU-20536), these would be very good to have for Buster. Then the version number update with the IANA tzdata update to 2019a [10] come. The latter is preprocessed to the source/data/in/icudt63l.dat file. In general the other changes you see are Windows only related changes. What I didn't mention is the hashtable speedup fix[6] and the IANA tzdata 2019a update which is preprocessed to the dat file. I know there's one more commit [11] (ICU-20558 Fix regression in DateTimePatternGenerator) but as I haven't seen it before it's also not in this unblock request (ie, not yet backported). How this should be handled? Do you need more information for icu-63.2-2 and/or can I use this fix as well and need to ask you only then for the unblock? Thanks for consideration, Laszlo/GCS [1] https://github.com/unicode-org/icu/commits/maint/maint-63 [2] https://github.com/unicode-org/icu/commit/46895456ad1b6660d17eaeba2c101600ad8d8eb8 [3] https://github.com/unicode-org/icu/commit/8baff8f03e07d8e02304d0c888d0bb21ad2eeb01 [4] https://github.com/unicode-org/icu/commit/6cbd62e59e30f73b444be89ea71fd74275ac53a4 [5] https://github.com/unicode-org/icu/commit/78d61f7cb9ecd98664d134da5909c8cc3dcee5a6 https://github.com/unicode-org/icu/commit/fced46c6a9404719482a27a29baa371809a64f37 https://github.com/unicode-org/icu/commit/2094a48c20193d8ecee70dc39a18f61bfda41f25 [6] https://github.com/unicode-org/icu/commit/38b882fe30e7105e691c0d48d35864724ea72979 [7] https://github.com/unicode-org/icu/commit/f3fa0d604ef6527a01dab96f4bfa3c5290127337 [8] https://github.com/unicode-org/icu/commit/0af507c4dabe517d9daa621d2d8c59c74b04af59 [9] https://github.com/unicode-org/icu/commit/838a14ee42336270260b54e11cd453a94b3c1a2d https://github.com/unicode-org/icu/commit/c948a1ae3132ab7e5af28f3321905dbaf97b0ce4 [10] https://github.com/unicode-org/icu/commit/4f715ae124c418a15a3fa4d8fb14f406576a7ee5 [11] https://github.com/unicode-org/icu/commit/5df4d7dfd8d77dd16aa3a0b398d50a22f4c85daa
Bug#928111: [pre-approval] unblock: icu/63.2-1
Control: tags -1 moreinfo Hi László, On 17-06-2019 22:05, László Böszörményi (GCS) wrote: > On Mon, Jun 17, 2019 at 9:50 PM Paul Gevers wrote: >> Do I understand correctly that what we are now getting with this version is: >> >> - Reiwa support >> - fix for the speed regression? >> >> Can you confirm that all the changes that I am seeing in your last diff >> (which were missing the debian/* changes by the way) are for those two >> issues? > Let me try to explain the current situation again. Current ICU version > in Sid is 63.2-2 which adds Reiwa support over the current version in > Buster. > The startup speed regression fix is _not_ present in this package > version. It would break the mentioned three source packages due to the > ABI change. The affected packages are binNMU-able, but the current > Chromium package is not ready to migrate to Buster and you can't > binNMU its Buster version. That's why it is only _planned_ for 63.2-3 > which is _not_ yet uploaded. > Hope I could describe the current situation better. There are quite a lot of changes in the diff, and only at the end we get to the Reiwa support (if I understand the code and comments at all). You haven't explained these other changes. If all we are now talking about is Reiwa support I will *not* unblock the current version of icu, as it introduces changes that are not targeted at fixing that. Paul signature.asc Description: OpenPGP digital signature
Bug#928111: [pre-approval] unblock: icu/63.2-1
Control: tags -1 -moreinfo On Mon, Jun 17, 2019 at 9:50 PM Paul Gevers wrote: > On 16-06-2019 11:20, László Böszörményi (GCS) wrote: [The ICU 63.2 upstream release] > > Last but not least fixed the startup slowness issue, changing a > > function signature as well. While this is a public function, I guess > > not meant to be used externally. It's an ABI break nevertheless > > without a soname bump. The good thing is that it's only used by the V8 > > JavaScript engine (being in Chromium). Just for the record, this mean > > chromium, nodejs and qtwebengine-opensource-src will need binNMUs once > > I can upload this change to Sid. > > Do I understand correctly that what we are now getting with this version is: > > - Reiwa support > - fix for the speed regression? > > Can you confirm that all the changes that I am seeing in your last diff > (which were missing the debian/* changes by the way) are for those two > issues? You asked for the patched sources difference as I understood - not to include noise with the patch changes under the debian directory. Let me try to explain the current situation again. Current ICU version in Sid is 63.2-2 which adds Reiwa support over the current version in Buster. The startup speed regression fix is _not_ present in this package version. It would break the mentioned three source packages due to the ABI change. The affected packages are binNMU-able, but the current Chromium package is not ready to migrate to Buster and you can't binNMU its Buster version. That's why it is only _planned_ for 63.2-3 which is _not_ yet uploaded. Hope I could describe the current situation better. Regards, Laszlo/GCS
Bug#928111: [pre-approval] unblock: icu/63.2-1
Control: tags -1 moreinfo Control: retitle -1 unblock: icu/63.2-2 Hi László, On 16-06-2019 11:20, László Böszörményi (GCS) wrote: >> Unfortunately, the state of chromium at this moment doesn't allow us to >> rebuild and have the package migrate to buster as chromium FTBFS on >> arm64 and the package is newer in unstable. You'll have to wait until >> that situation improves. > It improved but still not good. As it's irrelevant for now, let me > summarize the current status of ICU. > Unfortunately ICU 63.1 (which is in Buster already) have a startup > speed regression compared to previous versions. Chromium and > GraphicsMagick hit by this issue as I know, meaning these applications > start slower. > Then upstream released ICU 63.2 with three type of changes. First > security fixes that I've already backported to 63.1/Buster a while > ago. Then there's the Japanese new era "Reiwa" changes. This would be > nice to have for Buster. > Last but not least fixed the startup slowness issue, changing a > function signature as well. While this is a public function, I guess > not meant to be used externally. It's an ABI break nevertheless > without a soname bump. The good thing is that it's only used by the V8 > JavaScript engine (being in Chromium). Just for the record, this mean > chromium, nodejs and qtwebengine-opensource-src will need binNMUs once > I can upload this change to Sid. Do I understand correctly that what we are now getting with this version is: - Reiwa support - fix for the speed regression? Can you confirm that all the changes that I am seeing in your last diff (which were missing the debian/* changes by the way) are for those two issues? Paul signature.asc Description: OpenPGP digital signature
Bug#928111: [pre-approval] unblock: icu/63.2-1
Hi Paul,
On Sun, Jun 16, 2019 at 9:50 PM Paul Gevers wrote:
> On 16-06-2019 11:20, László Böszörményi (GCS) wrote:
> > The debdiff is larger for the following changes. The backported
> > security fixes are no longer under debian/patches but inline. The ABI
> > break, called the 'ICU-20250' issue upstream is reversed with a patch.
> > Then the s/63.1/63.2/ changes, etc.
>
> Can you please provide a diff between the patches-applied tree of the
> current buster version and a patches-applied tree of the current sid
> version?
Of course, attached. The diff size went down from 165 kB to 39 kB as
you see, even if the documentation and s/63.1/63.2/ changes are still
in as well.
Regards,
Laszlo/GCS
diff -Nur icu-63.1/readme.html icu-63.2/readme.html
--- icu-63.1/readme.html 2018-10-15 18:02:37.0 +
+++ icu-63.2/readme.html 2019-04-11 22:38:30.0 +
@@ -3,7 +3,7 @@
http://www.w3.org/1999/xhtml; xml:lang="en-US">
-ReadMe for ICU 63.1
+ReadMe for ICU 63.2
http://www.unicode.org/copyright.html"/>
diff -Nur icu-63.1/source/common/umutablecptrie.cpp icu-63.2/source/common/umutablecptrie.cpp
--- icu-63.1/source/common/umutablecptrie.cpp 2018-10-01 22:39:56.0 +
+++ icu-63.2/source/common/umutablecptrie.cpp 2019-06-16 20:23:58.0 +
@@ -60,6 +60,7 @@
constexpr int32_t INDEX_3_18BIT_BLOCK_LENGTH = UCPTRIE_INDEX_3_BLOCK_LENGTH + UCPTRIE_INDEX_3_BLOCK_LENGTH / 8;
class AllSameBlocks;
+class MixedBlocks;
class MutableCodePointTrie : public UMemory {
public:
@@ -92,8 +93,10 @@
void maskValues(uint32_t mask);
UChar32 findHighStart() const;
int32_t compactWholeDataBlocks(int32_t fastILimit, AllSameBlocks );
-int32_t compactData(int32_t fastILimit, uint32_t *newData, int32_t dataNullIndex);
-int32_t compactIndex(int32_t fastILimit, UErrorCode );
+int32_t compactData(
+int32_t fastILimit, uint32_t *newData, int32_t newDataCapacity,
+int32_t dataNullIndex, MixedBlocks , UErrorCode );
+int32_t compactIndex(int32_t fastILimit, MixedBlocks , UErrorCode );
int32_t compactTrie(int32_t fastILimit, UErrorCode );
uint32_t *index = nullptr;
@@ -548,28 +551,8 @@
}
}
-inline bool
-equalBlocks(const uint32_t *s, const uint32_t *t, int32_t length) {
-while (length > 0 && *s == *t) {
-++s;
-++t;
---length;
-}
-return length == 0;
-}
-
-inline bool
-equalBlocks(const uint16_t *s, const uint32_t *t, int32_t length) {
-while (length > 0 && *s == *t) {
-++s;
-++t;
---length;
-}
-return length == 0;
-}
-
-inline bool
-equalBlocks(const uint16_t *s, const uint16_t *t, int32_t length) {
+template
+bool equalBlocks(const UIntA *s, const UIntB *t, int32_t length) {
while (length > 0 && *s == *t) {
++s;
++t;
@@ -585,36 +568,6 @@
}
/** Search for an identical block. */
-int32_t findSameBlock(const uint32_t *p, int32_t pStart, int32_t length,
- const uint32_t *q, int32_t qStart, int32_t blockLength) {
-// Ensure that we do not even partially get past length.
-length -= blockLength;
-
-q += qStart;
-while (pStart <= length) {
-if (equalBlocks(p + pStart, q, blockLength)) {
-return pStart;
-}
-++pStart;
-}
-return -1;
-}
-
-int32_t findSameBlock(const uint16_t *p, int32_t pStart, int32_t length,
- const uint32_t *q, int32_t qStart, int32_t blockLength) {
-// Ensure that we do not even partially get past length.
-length -= blockLength;
-
-q += qStart;
-while (pStart <= length) {
-if (equalBlocks(p + pStart, q, blockLength)) {
-return pStart;
-}
-++pStart;
-}
-return -1;
-}
-
int32_t findSameBlock(const uint16_t *p, int32_t pStart, int32_t length,
const uint16_t *q, int32_t qStart, int32_t blockLength) {
// Ensure that we do not even partially get past length.
@@ -655,30 +608,9 @@
* Look for maximum overlap of the beginning of the other block
* with the previous, adjacent block.
*/
-int32_t getOverlap(const uint32_t *p, int32_t length,
- const uint32_t *q, int32_t qStart, int32_t blockLength) {
-int32_t overlap = blockLength - 1;
-U_ASSERT(overlap <= length);
-q += qStart;
-while (overlap > 0 && !equalBlocks(p + (length - overlap), q, overlap)) {
---overlap;
-}
-return overlap;
-}
-
-int32_t getOverlap(const uint16_t *p, int32_t length,
- const uint32_t *q, int32_t qStart, int32_t blockLength) {
-int32_t overlap = blockLength - 1;
-U_ASSERT(overlap <= length);
-q += qStart;
-while (overlap > 0 && !equalBlocks(p + (length - overlap), q, overlap)) {
---overlap;
-}
-return overlap;
-}
-
-int32_t getOverlap(const uint16_t *p, int32_t length,
- const uint16_t *q, int32_t qStart, int32_t blockLength) {
Bug#928111: [pre-approval] unblock: icu/63.2-1
Hi László, On 16-06-2019 11:20, László Böszörményi (GCS) wrote: > The debdiff is larger for the following changes. The backported > security fixes are no longer under debian/patches but inline. The ABI > break, called the 'ICU-20250' issue upstream is reversed with a patch. > Then the s/63.1/63.2/ changes, etc. Can you please provide a diff between the patches-applied tree of the current buster version and a patches-applied tree of the current sid version? Thanks. Paul signature.asc Description: OpenPGP digital signature
Bug#928111: [pre-approval] unblock: icu/63.2-1
Hi Lázló, On 08-06-2019 22:17, László Böszörményi (GCS) wrote: > While I think binNMU can be done for testing as well, I know that > would make things more complicated. We don't know why, but that currently doesn't work. We tried that for bug 928227. Paul signature.asc Description: OpenPGP digital signature
Bug#928111: [pre-approval] unblock: icu/63.2-1
Hi Paul, On Sat, Jun 8, 2019 at 9:57 PM Paul Gevers wrote: > On Sun, 5 May 2019 18:40:51 +0200 > =?UTF-8?B?TMOhc3psw7MgQsO2c3rDtnJtw6lueWkgKEdDUyk=?= wrote: > > Chromium is a bit bigger than me, but as I've experienced it's one of > > its pre-checks with an assert. As the size of the Unicode set is now > > bigger due to the addition of the new Japanese era it fails. Call it a > > lame check, but I tested this case vice-versa: built Chromium with ICU > > 63.2, then installed it with ICU 63.1 and it doesn't fail but working > > normally. > > Did you file a bug against chromium already about this? They should > improve the test to not fail like this in the future. If you haven't > done so, please file the bug ASAP. Nope, unfortunately ICU 63.2 breaks other things as well. I'm trying to find a solution for all the things, but couldn't finish it yet. I'm going to ping you again when I've a solution. > On top of that, your package will have to add a versioned Breaks on the > latest chromium that is built against the current version of icu. This > has to be set when the package can be uploaded to unstable. Now is not > the time yet, see below. I know unfortunately. I'll revisit this again when I've an updated ICU package (first for experimental of course). > Unfortunately, the state of chromium at this moment doesn't allow us to > rebuild and have the package migrate to buster as chromium FTBFS on > arm64 and the package is newer in unstable. You'll have to wait until > that situation improves. While I think binNMU can be done for testing as well, I know that would make things more complicated. Chromium is a big beast, kudos to Michael Gilbert for maintaining that. Cheers, Laszlo/GCS
Bug#928111: [pre-approval] unblock: icu/63.2-1
Control: tags -1 moreinfo Control: block -1 by 928097 Hi László, On Sun, 5 May 2019 18:40:51 +0200 =?UTF-8?B?TMOhc3psw7MgQsO2c3rDtnJtw6lueWkgKEdDUyk=?= wrote: > Chromium is a bit bigger than me, but as I've experienced it's one of > its pre-checks with an assert. As the size of the Unicode set is now > bigger due to the addition of the new Japanese era it fails. Call it a > lame check, but I tested this case vice-versa: built Chromium with ICU > 63.2, then installed it with ICU 63.1 and it doesn't fail but working > normally. Did you file a bug against chromium already about this? They should improve the test to not fail like this in the future. If you haven't done so, please file the bug ASAP. On top of that, your package will have to add a versioned Breaks on the latest chromium that is built against the current version of icu. This has to be set when the package can be uploaded to unstable. Now is not the time yet, see below. Unfortunately, the state of chromium at this moment doesn't allow us to rebuild and have the package migrate to buster as chromium FTBFS on arm64 and the package is newer in unstable. You'll have to wait until that situation improves. Paul signature.asc Description: OpenPGP digital signature
Bug#928111: [pre-approval] unblock: icu/63.2-1
On Sun, May 05, 2019 at 02:25:01PM +0200, Ivo De Decker wrote: > Secondly, the fact that chromium needs a rebuild suggest there is a change > that breaks something. > > I suggest you upload the new version to experimental. That way we can look at > the differences and people can test the new packages. And check the symbols and other parts of the ABI, as it would be important to understand why chromium needs a rebuild. -- regards, Mattia Rizzolo GPG Key: 66AE 2B4A FCCF 3F52 DA18 4D18 4B04 3FCD B944 4540 .''`. more about me: https://mapreri.org : :' : Launchpad user: https://launchpad.net/~mapreri `. `'` Debian QA page: https://qa.debian.org/developer.php?login=mattia `- signature.asc Description: PGP signature
Bug#928111: [pre-approval] unblock: icu/63.2-1
Control: tags -1 moreinfo Hi, On Sun, Apr 28, 2019 at 01:57:15PM +0200, László Böszörményi (GCS) wrote: > Hi Release Team, > > As Hideki Yamane noted on debian-release [1] Japanese new era is in > effect. ICU upstream released the 63.2 version to address this. The > packaging is ready to be uploaded. There are two things to consider. > First that it contains other (not related to the new era), stable > fixes as well. Including the two backported patches present in the > 63.1-6 package. > Second is that I've made local testing and only found a regression. > Chromium (the browser) needs to be binNMUed as otherwise it will crash > on startup. First of all, we can't make a statement on something like this without seeing the diff. Secondly, the fact that chromium needs a rebuild suggest there is a change that breaks something. This makes it very unlikely that this change is appropriate at this point in the freeze. Maybe targeted fixes on top of 63.1 would be better. Please do not upload 63.2 to unstable at this point. I suggest you upload the new version to experimental. That way we can look at the differences and people can test the new packages. Thanks, Ivo
Bug#928111: [pre-approval] unblock: icu/63.2-1
Package: release.debian.org Severity: normal User: release.debian@packages.debian.org Usertags: unblock Hi Release Team, As Hideki Yamane noted on debian-release [1] Japanese new era is in effect. ICU upstream released the 63.2 version to address this. The packaging is ready to be uploaded. There are two things to consider. First that it contains other (not related to the new era), stable fixes as well. Including the two backported patches present in the 63.1-6 package. Second is that I've made local testing and only found a regression. Chromium (the browser) needs to be binNMUed as otherwise it will crash on startup. Hope this can be allowed for Buster. Regards, Laszlo/GCS [1] https://lists.debian.org/debian-release/2019/04/msg00344.html
