Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Experiments with liquid 5.x #9229

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions .github/workflows/ci.yml
Expand Up @@ -14,16 +14,24 @@ jobs:
ci:
name: "Run Tests (${{ matrix.label }})"
runs-on: "ubuntu-latest"
env:
LIQUID_VERSION: ${{ matrix.liquid_version }}
strategy:
fail-fast: false
matrix:
include:
- label: Ruby 2.7
ruby_version: "2.7"
- label: Ruby 2.7 with Liquid v4
ruby_version: "2.7"
liquid_version: "~> 4.0"
Comment on lines +25 to +27

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- label: Ruby 2.7 with Liquid v4
ruby_version: "2.7"
liquid_version: "~> 4.0"

- label: Ruby 3.0
ruby_version: "3.0"
- label: Ruby 3.1.2
ruby_version: "3.1.2"
- label: Ruby 3.1.2 with Liquid v4
ruby_version: "3.1.2"
liquid_version: "~> 4.0"
Comment on lines +32 to +34

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- label: Ruby 3.1.2 with Liquid v4
ruby_version: "3.1.2"
liquid_version: "~> 4.0"

- label: JRuby 9.4.0.0
ruby_version: "jruby-9.4.0.0"
steps:
Expand Down
7 changes: 6 additions & 1 deletion Gemfile
Expand Up @@ -3,6 +3,12 @@
source "https://rubygems.org"
gemspec :name => "jekyll"

if ENV["LIQUID_VERSION"].to_s.empty?
gem "liquid", "~> 5.3.0"
else
gem "liquid", ENV["LIQUID_VERSION"]
end
Comment on lines +6 to +10

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if ENV["LIQUID_VERSION"].to_s.empty?
gem "liquid", "~> 5.3.0"
else
gem "liquid", ENV["LIQUID_VERSION"]
end
if RUBY_VERSION.to_f < 3.2.0
gem "liquid", "~> 4.0.3"
else
gem "liquid", "~> 5.4.0"
end


gem "rake", "~> 13.0"

group :development do
Expand Down Expand Up @@ -86,7 +92,6 @@ group :jekyll_optional_dependencies do

platforms :ruby, :mswin, :mingw, :x64_mingw do
gem "classifier-reborn", "~> 2.2"
gem "liquid-c", "~> 4.0"
gem "yajl-ruby", "~> 1.4"
end

Expand Down
4 changes: 4 additions & 0 deletions appveyor.yml
Expand Up @@ -21,6 +21,10 @@ environment:
- RUBY_FOLDER_VER: "27"
TZINFO_VERSION: "~> 2.0"
TEST_SUITE: "test"
- RUBY_FOLDER_VER: "27"
LIQUID_VERSION: "~> 4.0"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
LIQUID_VERSION: "~> 4.0"

TZINFO_VERSION: "~> 2.0"
TEST_SUITE: "test"
- RUBY_FOLDER_VER: "27"
TEST_SUITE: "default-site"
- RUBY_FOLDER_VER: "27"
Expand Down
2 changes: 1 addition & 1 deletion jekyll.gemspec
Expand Up @@ -40,7 +40,7 @@ Gem::Specification.new do |s|
s.add_runtime_dependency("jekyll-watch", "~> 2.0")
s.add_runtime_dependency("kramdown", "~> 2.3", ">= 2.3.1")
s.add_runtime_dependency("kramdown-parser-gfm", "~> 1.0")
s.add_runtime_dependency("liquid", "~> 4.0")
s.add_runtime_dependency("liquid", ">= 4.0", "< 5.4")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
s.add_runtime_dependency("liquid", ">= 4.0", "< 5.4")
if RUBY_VERSION.to_f < 3.2.0
s.add_runtime_dependency("liquid", "~> 4.0.3")
else
s.add_runtime_dependency("liquid", "~> 5.4.0")

Copy link
Author

@isimluk isimluk Dec 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with this as far as you are. But we need to depend on liquid < 5.4.

-    s.add_runtime_dependency("liquid", "~> 5.4.0")
+    s.add_runtime_dependency("liquid", "~> 5.3.0")

5.4 breaks further as you can see in the the test CI run on my repo: https://github.com/isimluk/jekyll/actions/runs/3777777021/jobs/6421776463

liquid 5.4.0 changed Registers to be a plain old ruby class whilst previously it has been Hash. I have a PR for that at Shopify/liquid#1663 but 5.4.0 remains unusable.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of just relying on RUBY_VERSION to determine the version of liquid to use?

I actually tend to think that is good idea. But how do you feel about the removal of liquid-c support that this PR currently brings ?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah setting the latest allowed version to 5.4 sounds reasonable to me.

Removing liquid-c is outside my understanding. It looks like it was introduced years ago, but wasn't actually being required until #7792 came along. Maybe it's fine to remove

s.add_runtime_dependency("mercenary", ">= 0.3.6", "< 0.5")
s.add_runtime_dependency("pathutil", "~> 0.9")
s.add_runtime_dependency("rouge", ">= 3.0", "< 5.0")
Expand Down
1 change: 1 addition & 0 deletions lib/jekyll.rb
Expand Up @@ -180,6 +180,7 @@ def sanitized_path(base_directory, questionable_path)

# Conditional optimizations
Jekyll::External.require_if_present("liquid/c")
require "jekyll/liquid_expr_method_literal" if Liquid::VERSION.start_with?("5.")
end
end

Expand Down
25 changes: 25 additions & 0 deletions lib/jekyll/liquid_expr_method_literal.rb
@@ -0,0 +1,25 @@
# frozen_string_literal: true

module Liquid
class Expression
class MethodLiteral
attr_reader :method_name, :to_s
alias_method :to_liquid, :to_s

def initialize(method_name, to_s)
@method_name = method_name
@to_s = to_s
end
end

PATCHED_LITERALS = Liquid::Expression::LITERALS.dup.tap do |obj|
obj["blank"] = MethodLiteral.new(:blank?, "").freeze
obj["empty"] = MethodLiteral.new(:empty?, "").freeze
end

remove_const :LITERALS
LITERALS = PATCHED_LITERALS.freeze

remove_const :PATCHED_LITERALS
end
end
3 changes: 2 additions & 1 deletion lib/jekyll/tags/include.rb
Expand Up @@ -254,7 +254,8 @@ def tag_includes_dirs(context)
end

def page_path(context)
page, site = context.registers.values_at(:page, :site)
page = context.registers.fetch(:page, nil)
site = context.registers.fetch(:site, nil)
return site.source unless page

site.in_source_dir File.dirname(resource_path(page, site))
Expand Down