Package: release.debian.org Severity: normal User: release.debian....@packages.debian.org Usertags: unblock X-Debbugs-Cc: z...@debian.org, by...@debian.org
Please unblock package opencc Summary of changes since testing/1.1.1+git20200624+ds2-5 + Enable upstream tests in package building. + Add autopkgtest + Backport 2 upstream patches to fix performance regression. [ Reason ] This fix is small, but the performance is improved a lot. As opencc is also used in generating Debian official website for Simplified and Traditional Chinese version, and a lot of text needs to be processed by opencc, so the performance matters. opencc is key package, so it needs manual unblock. [ Impact ] Without this patch, the performance drops a lot. [ Tests ] + Upstream unit and integration tests + Autopkgtest for installed library and tool + Manual tests + With opencc in testing: $ time opencc -c /usr/share/opencc/t2s.json -i <(printf "Open Chinese Convert 開放中文轉換\n%.0s" {1..50000}) -o /dev/null real 0m40.328s user 0m40.272s sys 0m0.105s + With opencc in unstable: $ time opencc -c /usr/share/opencc/t2s.json -i <(printf "Open Chinese Convert 開放中文轉換\n%.0s" {1..50000}) -o /dev/null real 0m0.556s user 0m0.551s sys 0m0.065s [ Risks ] + Patch is small + Key package [ Checklist ] [x] all changes are documented in the d/changelog [x] I reviewed all changes and I approve them [x] attach debdiff against the package in testing [ Other info ] The upstream patch was first backported in 1.1.1+git20200624+ds2-6, but the tests were not run, so Boyuan didn't notice the backport is incomplete. Then the backport was reverted in -7. After adding tests and autopkgtest, the patches are backported again in -10. unblock opencc/1.1.1+git20200624+ds2-10 Diff: Real effected code added are only 13 lines. + https://salsa.debian.org/debian/opencc/-/blob/debian/1.1.1+git20200624+ds2-10/debian/patches/0006-Fix-a-bug-in-the-calculation-of-DictGroup-keyMaxLeng.patch L28-L42 + https://salsa.debian.org/debian/opencc/-/blob/debian/1.1.1+git20200624+ds2-10/debian/patches/0007-Fix-a-severe-performance-bug-in-Conversion-Convert-t.patch L78-L80 Full diff: Also available at: https://salsa.debian.org/debian/opencc/-/compare/debian%2F1.1.1+git20200624+ds2-5...debian%2F1.1.1+git20200624+ds2-10 Some long test code added by upstream commit are skipped below. diff --git a/debian/changelog b/debian/changelog index eee8331..69db793 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,3 +1,48 @@ +opencc (1.1.1+git20200624+ds2-10) unstable; urgency=medium + + * Team upload. + * Upload to unstable. + * Backport patch to fix performance regression again. + Add + + 0006-Fix-a-bug-in-the-calculation-of-DictGroup-keyMaxLeng.patch + + 0007-Fix-a-severe-performance-bug-in-Conversion-Convert-t.patch + + -- Shengjing Zhu <z...@debian.org> Sun, 07 Mar 2021 14:20:40 +0800 + +opencc (1.1.1+git20200624+ds2-9) experimental; urgency=medium + + * Team upload. + * Remove unused command from autopkgtest scripts + * Add build-essential to autopkgtest + + -- Shengjing Zhu <z...@debian.org> Sun, 07 Mar 2021 00:54:22 +0800 + +opencc (1.1.1+git20200624+ds2-8) experimental; urgency=medium + + * Team upload. + * Enable test when building + * Add autopkgtest + + -- Shengjing Zhu <z...@debian.org> Sat, 06 Mar 2021 17:57:00 +0800 + +opencc (1.1.1+git20200624+ds2-7) unstable; urgency=high + + * Team upload. + * Drop debian/patches/0005 for now due to regression reported. + See also https://github.com/fcitx/fcitx5/issues/238 . + + -- Boyuan Yang <by...@debian.org> Fri, 05 Mar 2021 09:37:48 -0500 + +opencc (1.1.1+git20200624+ds2-6) unstable; urgency=high + + * Team upload. + * debian/patches/0005: Add upstream patch to fix severe performance + regression in `Conversion::Convert` that caused O(N^2) complexity. + * debian/rules: Disable parallel build to workaround some random + build error for now. + + -- Boyuan Yang <by...@debian.org> Sun, 28 Feb 2021 19:48:01 -0500 + opencc (1.1.1+git20200624+ds2-5) unstable; urgency=medium * Team upload. diff --git a/debian/control b/debian/control index 2eadce4..834dae0 100644 --- a/debian/control +++ b/debian/control @@ -13,6 +13,7 @@ Build-Depends: darts, debhelper-compat (= 13), doxygen <!nodoc>, + googletest <!nocheck>, libmarisa-dev, libtclap-dev, python3:any, diff --git a/debian/gbp.conf b/debian/gbp.conf new file mode 100644 index 0000000..cec628c --- /dev/null +++ b/debian/gbp.conf @@ -0,0 +1,2 @@ +[DEFAULT] +pristine-tar = True diff --git a/debian/patches/use-cmake-install-libdir.patch b/debian/patches/0001-use-cmake-install-libdir.patch similarity index 100% rename from debian/patches/use-cmake-install-libdir.patch rename to debian/patches/0001-use-cmake-install-libdir.patch diff --git a/debian/patches/0003-use-system-libraries.patch b/debian/patches/0002-use-system-libraries.patch similarity index 100% rename from debian/patches/0003-use-system-libraries.patch rename to debian/patches/0002-use-system-libraries.patch diff --git a/debian/patches/no-remote-images-when-reading-docs-on-disk.patch b/debian/patches/0004-no-remote-images-when-reading-docs-on-disk.patch similarity index 77% rename from debian/patches/no-remote-images-when-reading-docs-on-disk.patch rename to debian/patches/0004-no-remote-images-when-reading-docs-on-disk.patch index 3ea24ee..60b1bbc 100644 --- a/debian/patches/no-remote-images-when-reading-docs-on-disk.patch +++ b/debian/patches/0004-no-remote-images-when-reading-docs-on-disk.patch @@ -1,9 +1,17 @@ -Description: Don't fetch remote images when reading docs on disk - This fixes a privacy breach previously reported as Lintian warnings -Author: Gunnar Hjalmarsson <gunna...@ubuntu.com> +From: Gunnar Hjalmarsson <gunna...@ubuntu.com> +Date: Sat, 6 Mar 2021 17:45:20 +0800 +Subject: Don't fetch remote images when reading docs on disk + Forwarded: no Last-Update: 2021-01-14 +This fixes a privacy breach previously reported as Lintian warnings +--- + README.md | 8 ++++---- + 1 file changed, 4 insertions(+), 4 deletions(-) + +diff --git a/README.md b/README.md +index d941d6b..0663dcd 100644 --- a/README.md +++ b/README.md @@ -1,12 +1,12 @@ diff --git a/debian/patches/0005-Use-system-googletest.patch b/debian/patches/0005-Use-system-googletest.patch new file mode 100644 index 0000000..783d6bb --- /dev/null +++ b/debian/patches/0005-Use-system-googletest.patch @@ -0,0 +1,22 @@ +From: Shengjing Zhu <z...@debian.org> +Date: Sat, 6 Mar 2021 17:53:25 +0800 +Subject: Use system googletest + +Forwarded: not-needed +--- + CMakeLists.txt | 2 +- + 1 file changed, 1 insertion(+), 1 deletion(-) + +diff --git a/CMakeLists.txt b/CMakeLists.txt +index efb51ae..e26b934 100644 +--- a/CMakeLists.txt ++++ b/CMakeLists.txt +@@ -195,7 +195,7 @@ add_subdirectory(test) + ######## Testing + + if (ENABLE_GTEST) +- add_subdirectory(deps/gtest-1.11.0) ++ add_subdirectory(/usr/src/googletest/googletest ${CMAKE_BINARY_DIR}/googletest-build EXCLUDE_FROM_ALL) + enable_testing() + endif() + diff --git a/debian/patches/0006-Fix-a-bug-in-the-calculation-of-DictGroup-keyMaxLeng.patch b/debian/patches/0006-Fix-a-bug-in-the-calculation-of-DictGroup-keyMaxLeng.patch new file mode 100644 index 0000000..d89f9a3 --- /dev/null +++ b/debian/patches/0006-Fix-a-bug-in-the-calculation-of-DictGroup-keyMaxLeng.patch @@ -0,0 +1,97 @@ +From: Carbo Kuo <byv...@byvoid.com> +Date: Thu, 25 Feb 2021 20:48:50 +0900 +Subject: Fix a bug in the calculation of DictGroup::keyMaxLength. + +The length should be the maximum of all sub-dictionaries in the dictionary group. +--- + src/DictGroup.cpp | 16 ++++++++++++++-- + src/DictGroupTest.cpp | 32 ++++++++++++++++++++++++-------- + 2 files changed, 38 insertions(+), 10 deletions(-) + +diff --git a/src/DictGroup.cpp b/src/DictGroup.cpp +index 4ca9e33..c682e96 100644 +--- a/src/DictGroup.cpp ++++ b/src/DictGroup.cpp +@@ -1,7 +1,7 @@ + /* + * Open Chinese Convert + * +- * Copyright 2010-2014 Carbo Kuo <byv...@byvoid.com> ++ * Copyright 2010-2021 Carbo Kuo <byv...@byvoid.com> + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. +@@ -24,8 +24,20 @@ + + using namespace opencc; + ++namespace { ++ ++size_t GetKeyMaxLength(const std::list<DictPtr>& dicts) { ++ size_t keyMaxLength = 0; ++ for (const DictPtr& dict : dicts) { ++ keyMaxLength = std::max(keyMaxLength, dict->KeyMaxLength()); ++ } ++ return keyMaxLength; ++} ++ ++} // namespace ++ + DictGroup::DictGroup(const std::list<DictPtr>& _dicts) +- : keyMaxLength(0), dicts(_dicts) {} ++ : keyMaxLength(GetKeyMaxLength(_dicts)), dicts(_dicts) {} + + DictGroup::~DictGroup() {} + +diff --git a/src/DictGroupTest.cpp b/src/DictGroupTest.cpp +index 7003506..c91731e 100644 +--- a/src/DictGroupTest.cpp ++++ b/src/DictGroupTest.cpp <skip, upstream test file> diff --git a/debian/patches/0007-Fix-a-severe-performance-bug-in-Conversion-Convert-t.patch b/debian/patches/0007-Fix-a-severe-performance-bug-in-Conversion-Convert-t.patch new file mode 100644 index 0000000..a614830 --- /dev/null +++ b/debian/patches/0007-Fix-a-severe-performance-bug-in-Conversion-Convert-t.patch @@ -0,0 +1,188 @@ +From: Carbo Kuo <byv...@byvoid.com> +Date: Thu, 25 Feb 2021 21:13:38 +0900 +Subject: Fix a severe performance bug in `Conversion::Convert` that caused + O(N^2) complexity. + <skip, upstream log commit message> +This bug was reported in https://github.com/BYVoid/OpenCC/issues/478 and https://github.com/BYVoid/OpenCC/issues/517. +--- + src/Dict.hpp | 7 ++++ + src/benchmark/Performance.cpp | 77 ++++++++++++++++++++++++++++++++++--------- + 2 files changed, 69 insertions(+), 15 deletions(-) + +diff --git a/src/Dict.hpp b/src/Dict.hpp +index 461d6d2..1c81034 100644 +--- a/src/Dict.hpp ++++ b/src/Dict.hpp +@@ -49,6 +49,13 @@ public: + virtual Optional<const DictEntry*> MatchPrefix(const char* word, + size_t len) const; + ++ /** ++ * Matches the longest matched prefix of a word. ++ */ ++ Optional<const DictEntry*> MatchPrefix(const char* word) const { ++ return MatchPrefix(word, KeyMaxLength()); ++ } ++ + /** + * Matches the longest matched prefix of a word. + */ +diff --git a/src/benchmark/Performance.cpp b/src/benchmark/Performance.cpp +index cf8d3aa..d1b6468 100644 +--- a/src/benchmark/Performance.cpp ++++ b/src/benchmark/Performance.cpp +@@ -1,7 +1,26 @@ <skip, upstream performance test> diff --git a/debian/patches/series b/debian/patches/series index 74933a6..ad3c3d6 100644 --- a/debian/patches/series +++ b/debian/patches/series @@ -1,4 +1,7 @@ -use-cmake-install-libdir.patch -0003-use-system-libraries.patch +0001-use-cmake-install-libdir.patch +0002-use-system-libraries.patch 0003-data-Explicitly-use-python3.patch -no-remote-images-when-reading-docs-on-disk.patch +0004-no-remote-images-when-reading-docs-on-disk.patch +0005-Use-system-googletest.patch +0006-Fix-a-bug-in-the-calculation-of-DictGroup-keyMaxLeng.patch +0007-Fix-a-severe-performance-bug-in-Conversion-Convert-t.patch diff --git a/debian/rules b/debian/rules index 9bfc988..250a007 100755 --- a/debian/rules +++ b/debian/rules @@ -8,8 +8,10 @@ export DEB_BUILD_MAINT_OPTIONS = hardening=+all include /usr/share/dpkg/architecture.mk +# Disable parallel build to circumvent some random build error +# (needs further investigation) %: - dh $@ --buildsystem=cmake --with pkgkde_symbolshelper + dh $@ --buildsystem=cmake --with pkgkde_symbolshelper --no-parallel BUILD_OPTIONS = \ -DCMAKE_INSTALL_PREFIX=/usr \ @@ -23,6 +25,11 @@ else BUILD_OPTIONS += -DBUILD_DOCUMENTATION=OFF endif +ifeq (,$(filter nocheck,$(DEB_BUILD_OPTIONS))) +BUILD_OPTIONS += -DENABLE_GTEST=ON +else +BUILD_OPTIONS += -DENABLE_GTEST=OFF +endif override_dh_auto_configure: dh_auto_configure -- $(BUILD_OPTIONS) diff --git a/debian/tests/CMakeLists.txt b/debian/tests/CMakeLists.txt new file mode 100644 index 0000000..e6f88c7 --- /dev/null +++ b/debian/tests/CMakeLists.txt @@ -0,0 +1,24 @@ +cmake_minimum_required(VERSION 3.18) +project (opencc-integration CXX) +include (CTest) +enable_testing() + +find_package(PkgConfig REQUIRED) +find_program (OPENCC_TOOL opencc REQUIRED) +pkg_check_modules(OPENCC REQUIRED IMPORTED_TARGET opencc) + +add_definitions( + -DCMAKE_BINARY_DIR="${CMAKE_BINARY_DIR}" + -DCMAKE_SOURCE_DIR="${CMAKE_SOURCE_DIR}" + -DPROJECT_BINARY_PATH="${OPENCC_TOOL}" +) +make_directory(${CMAKE_BINARY_DIR}/test) +add_subdirectory(/usr/src/googletest/googletest ${CMAKE_BINARY_DIR}/googletest-build EXCLUDE_FROM_ALL) +set(UNITTESTS + CommandLineConvertTest +) +foreach(UNITTEST ${UNITTESTS}) + add_executable(${UNITTEST} ${UNITTEST}.cpp) + target_link_libraries(${UNITTEST} gtest gtest_main PkgConfig::OPENCC) + add_test(${UNITTEST} ${UNITTEST}) +endforeach(UNITTEST) diff --git a/debian/tests/CommandLineConvertTest.cpp b/debian/tests/CommandLineConvertTest.cpp new file mode 100644 index 0000000..6ca6068 --- /dev/null +++ b/debian/tests/CommandLineConvertTest.cpp @@ -0,0 +1,104 @@ +/* + * Open Chinese Convert + * + * Copyright 2015 Carbo Kuo <byv...@byvoid.com> + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include <fstream> + +#include "Common.hpp" +#include "gtest/gtest.h" + +namespace opencc { + +class CommandLineConvertTest : public ::testing::Test { +protected: + CommandLineConvertTest() { GetCurrentWorkingDirectory(); } + + virtual ~CommandLineConvertTest() { free(originalWorkingDirectory); } + + virtual void SetUp() { + ASSERT_NE("", CMAKE_BINARY_DIR); + ASSERT_NE("", CMAKE_SOURCE_DIR); + ASSERT_EQ(0, chdir("/usr/share/opencc/")); + } + + virtual void TearDown() { ASSERT_EQ(0, chdir(originalWorkingDirectory)); } + + std::string GetFileContents(const std::string& fileName) const { + std::ifstream fs(fileName); + EXPECT_TRUE(fs.is_open()); + const std::string content((std::istreambuf_iterator<char>(fs)), + (std::istreambuf_iterator<char>())); + fs.close(); + return content; + } + + void GetCurrentWorkingDirectory() { + originalWorkingDirectory = getcwd(nullptr, 0); + } + + const char* OpenccCommand() const { + return PROJECT_BINARY_PATH; + } + + const char* InputDirectory() const { + return CMAKE_SOURCE_DIR "/../../test/testcases/"; + } + + const char* OutputDirectory() const { return CMAKE_BINARY_DIR "/test/"; } + + const char* AnswerDirectory() const { + return CMAKE_SOURCE_DIR "/../../test/testcases/"; + } + + const char* ConfigurationDirectory() const { + return "/usr/share/opencc/"; + } + + std::string OutputFile(const char* config) const { + return std::string(OutputDirectory()) + config + ".out"; + } + + std::string AnswerFile(const char* config) const { + return std::string(AnswerDirectory()) + config + ".ans"; + } + + std::string TestCommand(const char* config) const { + return OpenccCommand() + std::string("") + " -i " + InputDirectory() + + config + ".in" + " -o " + OutputFile(config) + " -c " + + ConfigurationDirectory() + config + ".json"; + } + + char* originalWorkingDirectory; +}; + +class ConfigurationTest : public CommandLineConvertTest, + public ::testing::WithParamInterface<const char*> {}; + +TEST_P(ConfigurationTest, Convert) { + const char* config = GetParam(); + ASSERT_EQ(0, system(TestCommand(config).c_str())); + const std::string& output = GetFileContents(OutputFile(config)); + const std::string& answer = GetFileContents(AnswerFile(config)); + ASSERT_EQ(answer, output); +} + +INSTANTIATE_TEST_CASE_P(CommandLine, ConfigurationTest, + ::testing::Values("hk2s", "hk2t", "jp2t", "s2hk", "s2t", + "s2tw", "s2twp", "t2hk", "t2jp", "t2s", + "tw2s", "tw2sp", "tw2t")); + +} // namespace opencc diff --git a/debian/tests/README.md b/debian/tests/README.md new file mode 100644 index 0000000..12d6cec --- /dev/null +++ b/debian/tests/README.md @@ -0,0 +1,3 @@ +Fork from ../../test, please refresh this file if source has changed. + +Test with installed opencc tool and library. diff --git a/debian/tests/control b/debian/tests/control new file mode 100644 index 0000000..36df47b --- /dev/null +++ b/debian/tests/control @@ -0,0 +1,8 @@ +Tests: integration +Depends: + build-essential, + cmake, + googletest, + pkg-config, + @, +Restrictions: allow-stderr, diff --git a/debian/tests/integration b/debian/tests/integration new file mode 100755 index 0000000..c1f1746 --- /dev/null +++ b/debian/tests/integration @@ -0,0 +1,12 @@ +#!/bin/sh + +set -ex + +cd "$(dirname "$0")" +mkdir -p build +cd build +cmake .. +make +make test +cd .. +rm -rf build
signature.asc
Description: PGP signature