[Tarantool-patches] [PATCH v1 0/4] Fix app/digest and app/socket on 1.10

[Tarantool-patches] [PATCH v1 0/4] Fix app/digest and app/socket on 1.10

[Alexander V. Tikhonov]

Needed to port fixes from master for #4090 and commit the following:

1.  Before the patch unicode characters encoded with 4 bytes
    were always treated as non-printable and displayed as byte
    sequences (with 'binary' tag).
    With the patch, range of printable characters is extended and
    include characters encoded with 4 bytes.
    Currently it is: (old printable range) U (icu printable range).
    Corresponding changes are also made in tarantool/libyaml.
    
    Closes: #4090
    
    (cherry picked from commit cdf37876189fa005a350d6b69397348354bb2073)

2.  Bump libyaml
    
    Bumped libyaml with the following commits:
    
      7f41d551a44a9b947dc341dd5b6c8779b5d83f00 'Make sure libyaml is C89 compliant'
      f1d1e5e0a5f6e6adeebe0e2c5e95a6ee729426e4 'cmake: make sure yaml is built statically when used in tarantool'
      74a00faf5c4c8720149f7a726a5eda5e8fff86df 'Extend range of printable unicode characters'
    
    Needed for #4090

3.  build: remove libyaml from rpm/deb dependencies
    
    After we started using bundled version of libyaml by default (see commit
    47b91e90f2a4e23e70a1a6735af3de203ffd59f4), we can remove it from
    building dependencies for RPM and DEB packages.
    
    Closes #4442
    
    Reviewed-by: Alexander Turenko <alexander.turenko@tarantool.org>
    (cherry picked from commit 1d4e584a7b5a5570486d7dfa6df82e664f8e0f95)

4.  build: enable bundled libyaml for all systems.

    After we fixed bundled libyaml to correctly print 4-byte Unicode
    characters, it is no longer compatible with the upstream version, so
    enable building with bundled libyaml for every platform.
    This way the tests will pass.
    
    Follow-up #4090
    
    (cherry picked from commit 47b91e90f2a4e23e70a1a6735af3de203ffd59f4)

Github: https://github.com/tarantool/tarantool/tree/avtikhon/gh-5007-pack-yaml
Issue: https://github.com/tarantool/tarantool/issues/5007

Alexander V. Tikhonov (1):
  Bump libyaml

Ivan Koptelov (1):
  Extend range of printable unicode characters

Serge Petrenko (2):
  build: enable bundled libyaml for all systems.
  build: remove libyaml from rpm/deb dependencies

 apk/APKBUILD               |  2 --
 debian/control             |  3 +--
 debian/rules               |  1 -
 rpm/tarantool.spec         |  2 --
 snapcraft.yaml             |  1 -
 src/CMakeLists.txt         |  1 +
 src/util.c                 |  5 ++++-
 test/app-tap/yaml.test.lua | 10 +++++++++-
 third_party/libyaml        |  2 +-
 9 files changed, 16 insertions(+), 11 deletions(-)

-- 
2.17.1

[Tarantool-patches] [PATCH v1 1/4] build: enable bundled libyaml for all systems.

[Alexander V. Tikhonov]

From: Serge Petrenko <sergepetrenko@tarantool.org>

After we fixed bundled libyaml to correctly print 4-byte Unicode
characters, it is no longer compatible with the upstream version, so
enable building with bundled libyaml for every platform.
This way the tests will pass.

Follow-up #4090

(cherry picked from commit 47b91e90f2a4e23e70a1a6735af3de203ffd59f4)
---
 apk/APKBUILD       | 2 --
 debian/rules       | 1 -
 rpm/tarantool.spec | 1 -
 snapcraft.yaml     | 1 -
 4 files changed, 5 deletions(-)

diff --git a/apk/APKBUILD b/apk/APKBUILD
index e8ea5aa13..7d61aa4a5 100644
--- a/apk/APKBUILD
+++ b/apk/APKBUILD
@@ -23,10 +23,8 @@ build() {
     cd "$builddir"
 
     cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo \
-          -DENABLE_BUNDLED_LIBYAML:BOOL=OFF \
           -DENABLE_BACKTRACE:BOOL=ON \
           -DENABLE_DIST:BOOL=ON \
-          -DENABLE_BUNDLED_LIBYAML:BOOL=OFF \
           -DCMAKE_INSTALL_PREFIX=/usr \
           -DCMAKE_INSTALL_SYSCONFDIR=/etc \
           -DCMAKE_INSTALL_LOCALSTATEDIR=/var \
diff --git a/debian/rules b/debian/rules
index edfecfc33..904eaa719 100755
--- a/debian/rules
+++ b/debian/rules
@@ -15,7 +15,6 @@ DEB_CMAKE_EXTRA_FLAGS := \
 	-DCMAKE_INSTALL_LIBDIR=lib/$(DEB_HOST_MULTIARCH) \
 	-DCMAKE_INSTALL_SYSCONFDIR=/etc \
 	-DCMAKE_INSTALL_LOCALSTATEDIR=/var \
-	-DENABLE_BUNDLED_LIBYAML=OFF \
 	-DENABLE_DIST=ON \
 	-DWITH_SYSVINIT=ON \
 	-DWITH_SYSTEMD=$(WITH_SYSTEMD)
diff --git a/rpm/tarantool.spec b/rpm/tarantool.spec
index 8cd54c090..4b6ea6f9f 100644
--- a/rpm/tarantool.spec
+++ b/rpm/tarantool.spec
@@ -145,7 +145,6 @@ C and Lua/C modules.
 %cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo \
          -DCMAKE_INSTALL_LOCALSTATEDIR:PATH=%{_localstatedir} \
          -DCMAKE_INSTALL_SYSCONFDIR:PATH=%{_sysconfdir} \
-         -DENABLE_BUNDLED_LIBYAML:BOOL=OFF \
 %if %{with backtrace}
          -DENABLE_BACKTRACE:BOOL=ON \
 %else
diff --git a/snapcraft.yaml b/snapcraft.yaml
index 28429d2e7..b7e586789 100644
--- a/snapcraft.yaml
+++ b/snapcraft.yaml
@@ -25,7 +25,6 @@ parts:
         plugin: cmake
         configflags:
           - -DCMAKE_BUILD_TYPE=RelWithDebInfo
-          - -DENABLE_BUNDLED_LIBYAML=OFF
           - -DENABLE_DIST=OFF # Disable tarantoolctl, init scripts, etc.
         build-packages:
           - cmake
-- 
2.17.1

[Tarantool-patches] [PATCH v1 2/4] build: remove libyaml from rpm/deb dependencies

[Alexander V. Tikhonov]

From: Serge Petrenko <sergepetrenko@tarantool.org>

After we started using bundled version of libyaml by default (see commit
47b91e90f2a4e23e70a1a6735af3de203ffd59f4), we can remove it from
building dependencies for RPM and DEB packages.

Closes #4442

Reviewed-by: Alexander Turenko <alexander.turenko@tarantool.org>
(cherry picked from commit 1d4e584a7b5a5570486d7dfa6df82e664f8e0f95)
---
 debian/control     | 3 +--
 rpm/tarantool.spec | 1 -
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/debian/control b/debian/control
index 91d0b0e44..ea2e151fd 100644
--- a/debian/control
+++ b/debian/control
@@ -8,7 +8,6 @@ Build-Depends: cdbs (>= 0.4.100), debhelper (>= 9), dpkg-dev (>= 1.16.1~),
  cmake,
  libreadline-dev,
  libncurses5-dev,
- libyaml-dev,
  libssl-dev,
  libunwind-dev | libunwind8-dev | libunwind7-dev,
  libicu-dev,
@@ -54,7 +53,7 @@ Package: tarantool
 Architecture: i386 amd64 armhf arm64
 Priority: optional
 Depends: ${shlibs:Depends}, ${misc:Depends}, netbase, binutils, libgomp1,
- libyaml-0-2, openssl, tarantool-common (>= 1.7.5.46),
+  openssl, tarantool-common (>= 1.7.5.46),
 # libcurl dependencies (except ones we have already)
  zlib1g
 Replaces: tarantool-lts
diff --git a/rpm/tarantool.spec b/rpm/tarantool.spec
index 4b6ea6f9f..88b1d6b5c 100644
--- a/rpm/tarantool.spec
+++ b/rpm/tarantool.spec
@@ -15,7 +15,6 @@ BuildRequires: gcc-c++ >= 4.5
 BuildRequires: coreutils
 BuildRequires: sed
 BuildRequires: readline-devel
-BuildRequires: libyaml-devel
 BuildRequires: openssl-devel
 BuildRequires: libicu-devel
 #BuildRequires: msgpuck-devel
-- 
2.17.1

[Tarantool-patches] [PATCH v1 3/4] Bump libyaml

[Alexander V. Tikhonov]

Bumped libyaml with the following commits:

  7f41d551a44a9b947dc341dd5b6c8779b5d83f00 'Make sure libyaml is C89 compliant'
  f1d1e5e0a5f6e6adeebe0e2c5e95a6ee729426e4 'cmake: make sure yaml is built statically when used in tarantool'
  74a00faf5c4c8720149f7a726a5eda5e8fff86df 'Extend range of printable unicode characters'

Needed for #4090
---
 third_party/libyaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/third_party/libyaml b/third_party/libyaml
index 6bd4be1a7..7f41d551a 160000
--- a/third_party/libyaml
+++ b/third_party/libyaml
@@ -1 +1 @@
-Subproject commit 6bd4be1a7751d6022a413864666880bef8a87c3c
+Subproject commit 7f41d551a44a9b947dc341dd5b6c8779b5d83f00
-- 
2.17.1

[Tarantool-patches] [PATCH v1 4/4] Extend range of printable unicode characters

[Alexander V. Tikhonov]

From: Ivan Koptelov <ivan.koptelov@tarantool.org>

Before the patch unicode characters encoded with 4 bytes
were always treated as non-printable and displayed as byte
sequences (with 'binary' tag).
With the patch, range of printable characters is extended and
include characters encoded with 4 bytes.
Currently it is: (old printable range) U (icu printable range).
Corresponding changes are also made in tarantool/libyaml.

Closes: #4090

(cherry picked from commit cdf37876189fa005a350d6b69397348354bb2073)
---
 src/CMakeLists.txt         |  1 +
 src/util.c                 |  5 ++++-
 test/app-tap/yaml.test.lua | 10 +++++++++-
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index a8c515134..313d04e21 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -124,6 +124,7 @@ target_link_libraries(core
     ${LIBEIO_LIBRARIES}
     ${LIBCORO_LIBRARIES}
     ${MSGPUCK_LIBRARIES}
+    ${ICU_LIBRARIES}
     ${generic_libraries})
 
 add_library(stat STATIC rmean.c latency.c histogram.c)
diff --git a/src/util.c b/src/util.c
index efc5a64e7..ed43bda05 100644
--- a/src/util.c
+++ b/src/util.c
@@ -41,6 +41,8 @@
 #include <unistd.h>
 #include <limits.h>
 
+#include <unicode/utf8.h>
+#include <unicode/uchar.h>
 #include <msgpuck/msgpuck.h> /* mp_char2escape[] table */
 
 #include "say.h"
@@ -274,7 +276,8 @@ utf8_check_printable(const char *start, size_t length)
 		      (pointer[0] == 0xEF &&
 		       !(pointer[1] == 0xBB && pointer[2] == 0xBF) &&
 		       !(pointer[1] == 0xBF &&
-			 (pointer[2] == 0xBE || pointer[2] == 0xBF)))
+			 (pointer[2] == 0xBE || pointer[2] == 0xBF))) ||
+		      (u_isprint(value))
 		      )
 		    ) {
 			return 0;
diff --git a/test/app-tap/yaml.test.lua b/test/app-tap/yaml.test.lua
index c0eecea03..4669b6102 100755
--- a/test/app-tap/yaml.test.lua
+++ b/test/app-tap/yaml.test.lua
@@ -42,7 +42,7 @@ local function test_compact(test, s)
 end
 
 local function test_output(test, s)
-    test:plan(12)
+    test:plan(17)
     test:is(s.encode({true}), '---\n- true\n...\n', "encode for true")
     test:is(s.decode("---\nyes\n..."), true, "decode for 'yes'")
     test:is(s.encode({false}), '---\n- false\n...\n', "encode for false")
@@ -55,6 +55,14 @@ local function test_output(test, s)
         "encode for binary (2) - gh-354")
     test:is(s.encode("\xe0\x82\x85\x00"), '--- !!binary 4IKFAA==\n...\n',
         "encode for binary (3) - gh-1302")
+    -- gh-4090: some printable unicode characters displayed as byte sequences.
+    -- The following tests ensures that various 4-byte encoded unicode characters
+    -- displayed as expected.
+    test:is(s.encode("\xF0\x9F\x86\x98"), '--- 🆘\n...\n', "encode - gh-4090 (1)")
+    test:is(s.encode("\xF0\x9F\x84\xBD"), '--- 🄽\n...\n', "encode - gh-4090 (2)")
+    test:is(s.encode("\xF0\x9F\x85\xA9"), '--- 🅩\n...\n', "encode - gh-4090 (3)")
+    test:is(s.encode("\xF0\x9F\x87\xA6"), '--- 🇦\n...\n', "encode - gh-4090 (4)")
+    test:is(s.encode("\xF0\x9F\x88\xB2"), '--- 🈲\n...\n', "encode - gh-4090 (5)")
     -- gh-883: console can hang tarantool process
     local t = {}
     for i=0x8000,0xffff,1 do
-- 
2.17.1

Re: [Tarantool-patches] [PATCH v1 0/4] Fix app/digest and app/socket on 1.10

[Alexander Turenko]

Okay for bundling libyaml.

But I don't think we should cherry-pick #4090 fix to 1.10 together with
bundling.

On Thu, Jun 18, 2020 at 08:36:48AM +0300, Alexander V. Tikhonov wrote:
> Needed to port fixes from master for #4090 and commit the following:
> 
> 1.  Before the patch unicode characters encoded with 4 bytes
>     were always treated as non-printable and displayed as byte
>     sequences (with 'binary' tag).
>     With the patch, range of printable characters is extended and
>     include characters encoded with 4 bytes.
>     Currently it is: (old printable range) U (icu printable range).
>     Corresponding changes are also made in tarantool/libyaml.
>     
>     Closes: #4090
>     
>     (cherry picked from commit cdf37876189fa005a350d6b69397348354bb2073)

Not needed.

> 
> 2.  Bump libyaml
>     
>     Bumped libyaml with the following commits:
>     
>       7f41d551a44a9b947dc341dd5b6c8779b5d83f00 'Make sure libyaml is C89 compliant'
>       f1d1e5e0a5f6e6adeebe0e2c5e95a6ee729426e4 'cmake: make sure yaml is built statically when used in tarantool'

It is okay for 1.10. I would add tarantool-1.10 branch in libyaml.

>       74a00faf5c4c8720149f7a726a5eda5e8fff86df 'Extend range of printable unicode characters'

It should not be pushed to the branch for 1.10, IMHO.

>     
>     Needed for #4090

Nope, you work on #5007, not #4090. It is important to allow one
understand what is going on. Core of #4090 is 74a00faf5 ('Extend range
of printable unicode characters'), but I vote to exclude it here. If
we'll leave #4090 as the reason here, it will be misleading.

Please, write a few words, why this update is necessary (say, X distro
requires C89 support; installed package fails with 'unable to find the
dynamic library libyaml.so', so we should link it statically into
tarantool executable).

> 3.  build: remove libyaml from rpm/deb dependencies
>     
>     After we started using bundled version of libyaml by default (see commit
>     47b91e90f2a4e23e70a1a6735af3de203ffd59f4), we can remove it from
>     building dependencies for RPM and DEB packages.
>     
>     Closes #4442
>     
>     Reviewed-by: Alexander Turenko <alexander.turenko@tarantool.org>
>     (cherry picked from commit 1d4e584a7b5a5570486d7dfa6df82e664f8e0f95)

Why it is third? It should go after bundling.

Don't forget to add an issue into tarantool/doc repository when the
patch will land to 1.10 that will links to
https://github.com/tarantool/doc/issues/960

> 
> 4.  build: enable bundled libyaml for all systems.
> 
>     After we fixed bundled libyaml to correctly print 4-byte Unicode
>     characters, it is no longer compatible with the upstream version, so
>     enable building with bundled libyaml for every platform.
>     This way the tests will pass.
>     
>     Follow-up #4090
>     
>     (cherry picked from commit 47b91e90f2a4e23e70a1a6735af3de203ffd59f4)

I would add a few words after (cherry picked from ...), because the
original reason and the original issue is not actual for 1.10: let's
describe the actual reason and mention the actual issue.

Re: [Tarantool-patches] [PATCH v1 0/4] Fix app/digest and app/socket on 1.10

[Alexander V. Tikhonov]

Hi Alexander, thanks a lot for review and complete steps to fix. I've
made all your suggestions, please check my comments below.

On Thu, Jun 18, 2020 at 01:39:46PM +0300, Alexander Turenko wrote:
> Okay for bundling libyaml.
> 
> But I don't think we should cherry-pick #4090 fix to 1.10 together with
> bundling.
>

Ok.

> On Thu, Jun 18, 2020 at 08:36:48AM +0300, Alexander V. Tikhonov wrote:
> > Needed to port fixes from master for #4090 and commit the following:
> > 
> > 1.  Before the patch unicode characters encoded with 4 bytes
> >     were always treated as non-printable and displayed as byte
> >     sequences (with 'binary' tag).
> >     With the patch, range of printable characters is extended and
> >     include characters encoded with 4 bytes.
> >     Currently it is: (old printable range) U (icu printable range).
> >     Corresponding changes are also made in tarantool/libyaml.
> >     
> >     Closes: #4090
> >     
> >     (cherry picked from commit cdf37876189fa005a350d6b69397348354bb2073)
> 
> Not needed.
> 

Ok, removed.

> > 
> > 2.  Bump libyaml
> >     
> >     Bumped libyaml with the following commits:
> >     
> >       7f41d551a44a9b947dc341dd5b6c8779b5d83f00 'Make sure libyaml is C89 compliant'
> >       f1d1e5e0a5f6e6adeebe0e2c5e95a6ee729426e4 'cmake: make sure yaml is built statically when used in tarantool'
> 
> It is okay for 1.10. I would add tarantool-1.10 branch in libyaml.
> 

Sure, I've created tarantool-1.10 branch in libyaml and pointed the
commit to use it.

> >       74a00faf5c4c8720149f7a726a5eda5e8fff86df 'Extend range of printable unicode characters'
> 
> It should not be pushed to the branch for 1.10, IMHO.
> 

Ok, removed.

> >     
> >     Needed for #4090
> 
> Nope, you work on #5007, not #4090. It is important to allow one
> understand what is going on. Core of #4090 is 74a00faf5 ('Extend range
> of printable unicode characters'), but I vote to exclude it here. If
> we'll leave #4090 as the reason here, it will be misleading.
> 

Sure, changed the issue number.

> Please, write a few words, why this update is necessary (say, X distro
> requires C89 support; installed package fails with 'unable to find the
> dynamic library libyaml.so', so we should link it statically into
> tarantool executable).
> 

Actually found that this commit is not needed too.

> > 3.  build: remove libyaml from rpm/deb dependencies
> >     
> >     After we started using bundled version of libyaml by default (see commit
> >     47b91e90f2a4e23e70a1a6735af3de203ffd59f4), we can remove it from
> >     building dependencies for RPM and DEB packages.
> >     
> >     Closes #4442
> >     
> >     Reviewed-by: Alexander Turenko <alexander.turenko@tarantool.org>
> >     (cherry picked from commit 1d4e584a7b5a5570486d7dfa6df82e664f8e0f95)
> 
> Why it is third? It should go after bundling.
>

Right, put just after the bundling commit.

> Don't forget to add an issue into tarantool/doc repository when the
> patch will land to 1.10 that will links to
> https://github.com/tarantool/doc/issues/960
> 

Ok, I'll do it.

> > 
> > 4.  build: enable bundled libyaml for all systems.
> > 
> >     After we fixed bundled libyaml to correctly print 4-byte Unicode
> >     characters, it is no longer compatible with the upstream version, so
> >     enable building with bundled libyaml for every platform.
> >     This way the tests will pass.
> >     
> >     Follow-up #4090
> >     
> >     (cherry picked from commit 47b91e90f2a4e23e70a1a6735af3de203ffd59f4)
> 
> I would add a few words after (cherry picked from ...), because the
> original reason and the original issue is not actual for 1.10: let's
> describe the actual reason and mention the actual issue.

Sure, I've added the comments how the issues appeared in 1.10 repository
and described why we decided to fix it in this way.

Re: [Tarantool-patches] [PATCH v1 0/4] Fix app/digest and app/socket on 1.10

[Alexander Turenko]

> commit 6595e9fccfb3f52e9c551458cbe0285d651c812b
> Author: Alexander V. Tikhonov <avtikhon@tarantool.org>
> Date:   Thu Jun 18 01:55:58 2020 +0300
> 
>     Bump libyaml
>     
>     Set to use in libyaml tarantool-1.10 branch with the following
>     bumped commit:
>     
>       f1d1e5e0a5f6e6adeebe0e2c5e95a6ee729426e4 'cmake: make sure yaml is built statically when used in tarantool'

Nit: I would carry the line in the commit message.

Nit: I would use cherry-pick -x for picking commits from libyaml master
for tarantool-1.10 branch.

I guess a commit that fixes C89 build needed here too (ask Sergey P.
where he meet a problem with it).

Maybe it is better to place the libyaml update first, before enabling
bundling by default: this way looks more safe.

Other than that I have no objection.

LGTM. No need to re-review with me after the changes suggested above:
just carefully test it.

WBR, Alexander Turenko.

Re: [Tarantool-patches] [PATCH v1 0/4] Fix app/digest and app/socket on 1.10

[Kirill Yukhin]

Hello,

On 18 июн 08:36, Alexander V. Tikhonov wrote:
> Needed to port fixes from master for #4090 and commit the following:
> 
> 1.  Before the patch unicode characters encoded with 4 bytes
>     were always treated as non-printable and displayed as byte
>     sequences (with 'binary' tag).
>     With the patch, range of printable characters is extended and
>     include characters encoded with 4 bytes.
>     Currently it is: (old printable range) U (icu printable range).
>     Corresponding changes are also made in tarantool/libyaml.
>     
>     Closes: #4090
>     
>     (cherry picked from commit cdf37876189fa005a350d6b69397348354bb2073)
> 
> 2.  Bump libyaml
>     
>     Bumped libyaml with the following commits:
>     
>       7f41d551a44a9b947dc341dd5b6c8779b5d83f00 'Make sure libyaml is C89 compliant'
>       f1d1e5e0a5f6e6adeebe0e2c5e95a6ee729426e4 'cmake: make sure yaml is built statically when used in tarantool'
>       74a00faf5c4c8720149f7a726a5eda5e8fff86df 'Extend range of printable unicode characters'
>     
>     Needed for #4090
> 
> 3.  build: remove libyaml from rpm/deb dependencies
>     
>     After we started using bundled version of libyaml by default (see commit
>     47b91e90f2a4e23e70a1a6735af3de203ffd59f4), we can remove it from
>     building dependencies for RPM and DEB packages.
>     
>     Closes #4442
>     
>     Reviewed-by: Alexander Turenko <alexander.turenko@tarantool.org>
>     (cherry picked from commit 1d4e584a7b5a5570486d7dfa6df82e664f8e0f95)
> 
> 4.  build: enable bundled libyaml for all systems.
> 
>     After we fixed bundled libyaml to correctly print 4-byte Unicode
>     characters, it is no longer compatible with the upstream version, so
>     enable building with bundled libyaml for every platform.
>     This way the tests will pass.
>     
>     Follow-up #4090
>     
>     (cherry picked from commit 47b91e90f2a4e23e70a1a6735af3de203ffd59f4)
> 
> Github: https://github.com/tarantool/tarantool/tree/avtikhon/gh-5007-pack-yaml
> Issue: https://github.com/tarantool/tarantool/issues/5007
> 
> Alexander V. Tikhonov (1):
>   Bump libyaml
> 
> Ivan Koptelov (1):
>   Extend range of printable unicode characters
> 
> Serge Petrenko (2):
>   build: enable bundled libyaml for all systems.
>   build: remove libyaml from rpm/deb dependencies

I've checked updated patchset into 1.10.

--
Regards, Kirill Yukhin