Bug 27673 - Replace YAML with YAML::XS
Summary: Replace YAML with YAML::XS
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Joonas Kylmälä
URL:
Keywords: dependency
Depends on:
Blocks: 22824 23583 26633 28110
  Show dependency treegraph
 
Reported: 2021-02-10 14:28 UTC by Jonathan Druart
Modified: 2021-04-29 13:34 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Replaces uses of YAML module with more widely supported YAML::XS module.
Version(s) released in:
21.05.00


Attachments
Bug 27673: Replace YAML with YAML::XS (25.74 KB, patch)
2021-02-10 14:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27673: Replace YAML with YAML::XS (25.81 KB, patch)
2021-02-10 15:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27673: Fix encoding issues (9.04 KB, patch)
2021-02-11 09:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27673: Rename C4::Circulation decode to _decode (1.38 KB, patch)
2021-02-11 09:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27673: Fix encoding issues - Dump (3.04 KB, patch)
2021-02-11 09:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27673: Replace YAML with YAML::XS (25.81 KB, patch)
2021-02-11 14:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27673: Fix encoding issues (9.11 KB, patch)
2021-02-11 14:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27673: Rename C4::Circulation decode to _decode (1.46 KB, patch)
2021-02-11 14:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27673: Fix encoding issues - Dump (3.11 KB, patch)
2021-02-11 14:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27673: Replace YAML with YAML::XS (25.94 KB, patch)
2021-02-11 15:32 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 27673: Fix encoding issues (9.16 KB, patch)
2021-02-11 15:32 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 27673: Rename C4::Circulation decode to _decode (1.59 KB, patch)
2021-02-11 15:32 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 27673: Fix encoding issues - Dump (3.16 KB, patch)
2021-02-11 15:32 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 27673: (QA follow-up) Remove unused YAML:XS import (899 bytes, patch)
2021-02-11 15:32 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 27673: (QA follow-up) Remove unused YAML::XS import (900 bytes, patch)
2021-02-11 16:46 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 27673: (follow-up) Fix encoding issues - Dump (1.37 KB, patch)
2021-02-12 14:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27673: Fix t/Context.t (982 bytes, patch)
2021-02-17 08:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27673: remove libyaml-syck-perl, and lower libyaml-libyaml-perl to 0.41 (for u16) (962 bytes, patch)
2021-02-18 13:47 UTC, Mason James
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2021-02-10 14:28:08 UTC
From tht YAML pod:

"""
This module has been released to CPAN as YAML::Old, and soon YAML.pm will be changed to just be a frontend interface module for all the various Perl YAML implementation modules, including YAML::Old.

If you want robust and fast YAML processing using the normal Dump/Load API, please consider switching to YAML::XS. It is by far the best Perl module for YAML at this time. It requires that you have a C compiler, since it is written in C.
"""

See also
https://gitlab.com/koha-community/qa-test-tools/-/merge_requests/35
Comment 1 Jonathan Druart 2021-02-10 14:32:26 UTC
Created attachment 116670 [details] [review]
Bug 27673: Replace YAML with YAML::XS

From tht YAML pod:

"""
This module has been released to CPAN as YAML::Old, and soon YAML.pm will be changed to just be a frontend interface module for all the various Perl YAML implementation modules, including YAML::Old.

If you want robust and fast YAML processing using the normal Dump/Load API, please consider switching to YAML::XS. It is by far the best Perl module for YAML at this time. It requires that you have a C compiler, since it is written in C.
"""

See also
https://gitlab.com/koha-community/qa-test-tools/-/merge_requests/35

Test plan:
Try some place where YAML::XS is not used and confirm that it works
correctly

QA note: This patch removes some uses of YAML that were not useful
Comment 2 Kyle M Hall 2021-02-10 15:05:59 UTC
Created attachment 116675 [details] [review]
Bug 27673: Replace YAML with YAML::XS

From tht YAML pod:

"""
This module has been released to CPAN as YAML::Old, and soon YAML.pm will be changed to just be a frontend interface module for all the various Perl YAML implementation modules, including YAML::Old.

If you want robust and fast YAML processing using the normal Dump/Load API, please consider switching to YAML::XS. It is by far the best Perl module for YAML at this time. It requires that you have a C compiler, since it is written in C.
"""

See also
https://gitlab.com/koha-community/qa-test-tools/-/merge_requests/35

Test plan:
Try some place where YAML::XS is not used and confirm that it works
correctly

QA note: This patch removes some uses of YAML that were not useful

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 3 Jonathan Druart 2021-02-11 09:57:50 UTC
Created attachment 116694 [details] [review]
Bug 27673: Fix encoding issues

There is a difference between YAML::Load and YAML::XS::Load
From YAML::XS pod:
"YAML::XS only deals with streams of utf8 octets"

Test plan:
We are going to test 1 occurence and QA will confirm others don't
contain typos.
0. Don't apply the patches
1. Create a new itemtype with code=❤️
2. Create a new item using this itemtype (to biblionumber=1 will work)
3. Fill OpacHiddenItems with
 itype: [❤️]
4. Search for "street shuffle" or any terms that will return the biblio
Notice that the item is there (there is an error in logs)
5. Apply the patches
6. Repeat 4 and confirm that the item is now hidden
Comment 4 Jonathan Druart 2021-02-11 09:57:53 UTC
Created attachment 116695 [details] [review]
Bug 27673: Rename C4::Circulation decode to _decode

To prevent conflict with Encode::encode
Comment 5 Jonathan Druart 2021-02-11 09:57:55 UTC
Created attachment 116696 [details] [review]
Bug 27673: Fix encoding issues - Dump

Same as Load, but for Dump.

Test plan:
Edit ES mappings, replace withdrawn's label with "withdrawn ✔️ ❤️ ★"
Export the mappings
  perl misc/search_tools/export_elasticsearch_mappings.pl > admin/searchengine/elasticsearch/mappings.yaml
Reset mappings from the UI
=> Notice that the label is correct
Comment 6 Kyle M Hall 2021-02-11 14:30:24 UTC
Created attachment 116737 [details] [review]
Bug 27673: Replace YAML with YAML::XS

From tht YAML pod:

"""
This module has been released to CPAN as YAML::Old, and soon YAML.pm will be changed to just be a frontend interface module for all the various Perl YAML implementation modules, including YAML::Old.

If you want robust and fast YAML processing using the normal Dump/Load API, please consider switching to YAML::XS. It is by far the best Perl module for YAML at this time. It requires that you have a C compiler, since it is written in C.
"""

See also
https://gitlab.com/koha-community/qa-test-tools/-/merge_requests/35

Test plan:
Try some place where YAML::XS is not used and confirm that it works
correctly

QA note: This patch removes some uses of YAML that were not useful

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 7 Kyle M Hall 2021-02-11 14:30:54 UTC
Created attachment 116738 [details] [review]
Bug 27673: Fix encoding issues

There is a difference between YAML::Load and YAML::XS::Load
From YAML::XS pod:
"YAML::XS only deals with streams of utf8 octets"

Test plan:
We are going to test 1 occurence and QA will confirm others don't
contain typos.
0. Don't apply the patches
1. Create a new itemtype with code=❤️
2. Create a new item using this itemtype (to biblionumber=1 will work)
3. Fill OpacHiddenItems with
 itype: [❤️]
4. Search for "street shuffle" or any terms that will return the biblio
Notice that the item is there (there is an error in logs)
5. Apply the patches
6. Repeat 4 and confirm that the item is now hidden

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 8 Kyle M Hall 2021-02-11 14:30:57 UTC
Created attachment 116739 [details] [review]
Bug 27673: Rename C4::Circulation decode to _decode

To prevent conflict with Encode::encode

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 9 Kyle M Hall 2021-02-11 14:31:00 UTC
Created attachment 116740 [details] [review]
Bug 27673: Fix encoding issues - Dump

Same as Load, but for Dump.

Test plan:
Edit ES mappings, replace withdrawn's label with "withdrawn ✔️ ❤️ ★"
Export the mappings
  perl misc/search_tools/export_elasticsearch_mappings.pl > admin/searchengine/elasticsearch/mappings.yaml
Reset mappings from the UI
=> Notice that the label is correct

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 10 Joonas Kylmälä 2021-02-11 15:32:27 UTC
Created attachment 116762 [details] [review]
Bug 27673: Replace YAML with YAML::XS

From tht YAML pod:

"""
This module has been released to CPAN as YAML::Old, and soon YAML.pm will be changed to just be a frontend interface module for all the various Perl YAML implementation modules, including YAML::Old.

If you want robust and fast YAML processing using the normal Dump/Load API, please consider switching to YAML::XS. It is by far the best Perl module for YAML at this time. It requires that you have a C compiler, since it is written in C.
"""

See also
https://gitlab.com/koha-community/qa-test-tools/-/merge_requests/35

Test plan:
Try some place where YAML::XS is not used and confirm that it works
correctly

QA note: This patch removes some uses of YAML that were not useful

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 11 Joonas Kylmälä 2021-02-11 15:32:30 UTC
Created attachment 116763 [details] [review]
Bug 27673: Fix encoding issues

There is a difference between YAML::Load and YAML::XS::Load
From YAML::XS pod:
"YAML::XS only deals with streams of utf8 octets"

Test plan:
We are going to test 1 occurence and QA will confirm others don't
contain typos.
0. Don't apply the patches
1. Create a new itemtype with code=❤️
2. Create a new item using this itemtype (to biblionumber=1 will work)
3. Fill OpacHiddenItems with
 itype: [❤️]
4. Search for "street shuffle" or any terms that will return the biblio
Notice that the item is there (there is an error in logs)
5. Apply the patches
6. Repeat 4 and confirm that the item is now hidden

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 12 Joonas Kylmälä 2021-02-11 15:32:34 UTC
Created attachment 116764 [details] [review]
Bug 27673: Rename C4::Circulation decode to _decode

To prevent conflict with Encode::encode

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 13 Joonas Kylmälä 2021-02-11 15:32:37 UTC
Created attachment 116765 [details] [review]
Bug 27673: Fix encoding issues - Dump

Same as Load, but for Dump.

Test plan:
Edit ES mappings, replace withdrawn's label with "withdrawn ✔️ ❤️ ★"
Export the mappings
  perl misc/search_tools/export_elasticsearch_mappings.pl > admin/searchengine/elasticsearch/mappings.yaml
Reset mappings from the UI
=> Notice that the label is correct

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 14 Joonas Kylmälä 2021-02-11 15:32:40 UTC
Created attachment 116766 [details] [review]
Bug 27673: (QA follow-up) Remove unused YAML:XS import

The last use of this module was removed in commit
"Bug 27673: Fix encoding issues - Dump".

Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 15 Joonas Kylmälä 2021-02-11 16:03:35 UTC
The QA script gave some warning about C4::Circulation but it was because the patches modified the POD that was already broken. IMHO, out of scope for this bug report.
Comment 16 Joonas Kylmälä 2021-02-11 16:46:37 UTC
Created attachment 116778 [details] [review]
Bug 27673: (QA follow-up) Remove unused YAML::XS import

The last use of this module was removed in commit
"Bug 27673: Fix encoding issues - Dump".

Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 17 Joonas Kylmälä 2021-02-11 16:47:45 UTC
The last QA follow-up from me had typo in the commit title: "YAML:XS" -> "YAML::XS". Attached fixed version.
Comment 18 Jonathan Druart 2021-02-12 14:37:56 UTC
Created attachment 116835 [details] [review]
Bug 27673: (follow-up) Fix encoding issues - Dump
Comment 19 Jonathan Druart 2021-02-12 14:38:32 UTC
(In reply to Jonathan Druart from comment #18)
> Created attachment 116835 [details] [review] [review]
> Bug 27673: (follow-up) Fix encoding issues - Dump

Julian caught that, thanks!
Comment 20 Jonathan Druart 2021-02-16 13:55:16 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 21 Jonathan Druart 2021-02-17 08:22:42 UTC
Created attachment 116929 [details] [review]
Bug 27673: Fix t/Context.t

YAML::XS is not failing on that string, it generates
  'uno - dos' => "asd"

This new string is not YAML valid because of
"found a tab character that violate intendation"
Comment 22 Jonathan Druart 2021-02-17 08:26:17 UTC
(In reply to Jonathan Druart from comment #21)
> Created attachment 116929 [details] [review] [review]
> Bug 27673: Fix t/Context.t
> 
> YAML::XS is not failing on that string, it generates
>   'uno - dos' => "asd"
> 
> This new string is not YAML valid because of
> "found a tab character that violate intendation"

Patch pushed to master.
Comment 23 Jonathan Druart 2021-02-17 15:25:22 UTC
U16 is failing because of this, YAML::XS is missing.

Don't backport!
Comment 24 Joonas Kylmälä 2021-02-17 15:33:11 UTC
(In reply to Jonathan Druart from comment #23)
> U16 is failing because of this, YAML::XS is missing.
> 
> Don't backport!

Very strange because https://packages.ubuntu.com/xenial/libyaml-libyaml-perl lists it as being available on Ubuntu 16.04 / Xenial. Could the Jenkins/Docker setup have some problem relating to which packages get installed?
Comment 25 Joonas Kylmälä 2021-02-17 15:37:02 UTC
(In reply to Joonas Kylmälä from comment #24)
> Very strange because https://packages.ubuntu.com/xenial/libyaml-libyaml-perl
> lists it as being available on Ubuntu 16.04 / Xenial. Could the
> Jenkins/Docker setup have some problem relating to which packages get
> installed?

I think it is because in our debian control file we don't specify libyaml-libyaml-perl to be installed explicitly, we only install libyaml-perl which has two alternative recommendations (see https://packages.ubuntu.com/xenial/libyaml-perl):

- libyaml-syck-perl
- libyaml-libyaml-perl

and in our control file we explicitly say we want libyaml-syck-perl so the other one doesn't get installed most likely because of that.
Comment 26 Jonathan Druart 2021-02-17 15:40:25 UTC
Right, thanks Joonas.
Comment 27 Mason James 2021-02-18 13:47:14 UTC
Created attachment 116990 [details] [review]
Bug 27673: remove libyaml-syck-perl, and lower libyaml-libyaml-perl to 0.41 (for u16)

1/ apply patch
2/ build koha package
3/ confirm that libyaml-libyaml-perl package is installed, and
    libyaml-syck-perl is not
Comment 28 Joonas Kylmälä 2021-02-18 13:58:15 UTC
(In reply to Mason James from comment #27)
> Created attachment 116990 [details] [review] [review]
> Bug 27673: remove libyaml-syck-perl, and lower libyaml-libyaml-perl to 0.41
> (for u16)
> 
> 1/ apply patch
> 2/ build koha package
> 3/ confirm that libyaml-libyaml-perl package is installed, and
>     libyaml-syck-perl is not

I think libyaml-syck-perl cannot be removed because bug 22824 is not pushed.
Comment 29 Fridolin Somers 2021-02-22 10:04:22 UTC
Enhancement will not be pushed to 20.11.x
Comment 30 Jonathan Druart 2021-03-01 14:24:23 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 31 Jonathan Druart 2021-03-01 14:27:05 UTC
Mason, I pushed your follow-up, but did you see Joonas's comment about it?
Comment 32 Martin Renvoize 2021-03-04 07:05:03 UTC
Great to see this one in.. 😀, I'm going to spend some time on bug 22824 today. It'll be fantastic to get Koha onto one consistent yaml handler :)
Comment 33 Mason James 2021-03-26 04:51:31 UTC
(In reply to Joonas Kylmälä from comment #28)
> (In reply to Mason James from comment #27)
> > Created attachment 116990 [details] [review] [review] [review]
> > Bug 27673: remove libyaml-syck-perl, and lower libyaml-libyaml-perl to 0.41
> > (for u16)
> > 
> > 1/ apply patch
> > 2/ build koha package
> > 3/ confirm that libyaml-libyaml-perl package is installed, and
> >     libyaml-syck-perl is not
> 
> I think libyaml-syck-perl cannot be removed because bug 22824 is not pushed.

update:  bug 22824 is now pushed to master