Shopify / liquid

Liquid markup language. Safe, customer facing template language for flexible web apps.
https://shopify.github.io/liquid/
MIT License
11.13k stars 1.39k forks source link

Test fails with Fedora glibc -2.38.9000-20.fc40 and onwards, perhaps due to qsort behavior change #1759

Closed mtasaka closed 8 months ago

mtasaka commented 12 months ago

With ruby 3.3.0dev (2023-11-25 master f6b292b5ca) [x86_64-linux] : https://github.com/ruby/ruby/commit/fb7add495454322ea00efa7549feb957cb1ca538 , test suite for liquid git: https://github.com/Shopify/liquid/commit/e3dcc75ab533eb177cabf3df16ed27c1a21fc33c fails like

$ ruby -I"lib:test" -e 'Dir.glob "./test/**/*_test.rb", &method(:require)' 
Run options: --seed 59170

# Running:

Finished in 0.920206s, 848.7235 runs/s, 2050.6289 assertions/s.

  1) Failure:
StandardFiltersTest#test_sort_natural_when_property_is_sometimes_missing_puts_nils_last [/builddir/build/GIT/liquid/test/integration/standard_filter_test.rb:337]:
--- expected
+++ actual
@@ -1 +1 @@
-[{"price"=>"1", "handle"=>"gamma"}, {"price"=>2, "handle"=>"epsilon"}, {"price"=>"4", "handle"=>"alpha"}, {"handle"=>"delta"}, {"handle"=>"beta"}]
+[{"price"=>"1", "handle"=>"gamma"}, {"price"=>2, "handle"=>"epsilon"}, {"price"=>"4", "handle"=>"alpha"}, {"handle"=>"beta"}, {"handle"=>"delta"}]

781 runs, 1887 assertions, 1 failures, 0 errors, 0 skips
mtasaka commented 12 months ago

Well, it turned out that this is not due to ruby 3.3.0dev side change, but due to glibc change.

Using Fedora 40: liquid test

glibc-2.38.9000-20.fc40 changelog says:

* Sat Nov 11 2023 Florian Weimer <fweimer@redhat.com> - 2.38.9000-20
- Drop glibc-rh2248502-*.patch, workaround applied upstream
- Auto-sync with upstream branch master,
  commit d1dcb565a1fb5829f9476a1438c30eccc4027d04:
- Fix type typo in “String/Array Conventions” doc
- stdlib: Avoid element self-comparisons in qsort (#2248502)
- elf: Add glibc.mem.decorate_maps tunable
- linux: Decorate __libc_fatal error buffer
- assert: Decorate error message buffer
- malloc: Decorate malloc maps
- nptl: Decorate thread stack on pthread_create
- support: Add support_set_vma_name
- linux: Add PR_SET_VMA_ANON_NAME support

Looking at this changelog, I guess there is surely some implementation change on glibc qsort, and perhaps this change affects liquid.

mtasaka commented 11 months ago

So looking at: https://github.com/Shopify/liquid/pull/1476 , nil_safe_compare is changed so that the comparison between nil <=> nil returns 0 , not nil (i.e. nil and nil is equal), and corespondingly test_sort_when_property_is_sometimes_missing_puts_nils_last is changed.

So although the test error of this report happened with development stage glibc 2.38.9000 and does not see with glibc 2.38, I think we should also modify nil_safe_casecmp to does the consistent behavior as nil_safe_compare, i.e. the comparison between nil <=> nil should return 0, and modify test_sort_natural_when_property_is_sometimes_missing_puts_nils_last accordingly.