|
Description
Jonathan Druart
2021-02-10 14:28:08 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 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> 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 Created attachment 116695 [details] [review] Bug 27673: Rename C4::Circulation decode to _decode To prevent conflict with Encode::encode 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 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> 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> 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> 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> 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> 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> 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> 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> 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> 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. 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> The last QA follow-up from me had typo in the commit title: "YAML:XS" -> "YAML::XS". Attached fixed version. Created attachment 116835 [details] [review] Bug 27673: (follow-up) Fix encoding issues - Dump (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! Pushed to master for 21.05, thanks to everybody involved! 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" (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. U16 is failing because of this, YAML::XS is missing. Don't backport! (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? (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. Right, thanks Joonas. 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 (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. Enhancement will not be pushed to 20.11.x Pushed to master for 21.05, thanks to everybody involved! Mason, I pushed your follow-up, but did you see Joonas's comment about it? Great to see this one in.. [U+1F600], I'm going to spend some time on bug 22824 today. It'll be fantastic to get Koha onto one consistent yaml handler :) (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 We just updated to 20.11.12 and noticed that libyaml-libyaml-perl was missing. This was marked only for 21.05, but is it possible a backport could require it? (In reply to Katrin Fischer from comment #34) > We just updated to 20.11.12 and noticed that libyaml-libyaml-perl was > missing. This was marked only for 21.05, but is it possible a backport could > require it? It was cleared up on IRC: http://irc.koha-community.org/koha/2021-12-07#i_2392844 |