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

["Alexander V. Tikhonov" <avtikhon@tarantool.org>]

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.