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

Update sass related tests for jekyll-sass-converter 3.x #9223

Merged
merged 1 commit into from
Dec 22, 2022
Merged

Update sass related tests for jekyll-sass-converter 3.x #9223

merged 1 commit into from
Dec 22, 2022

Conversation

ntkme
Copy link
Contributor

@ntkme ntkme commented Dec 21, 2022

This is a 🐛 bug fix.

Summary

jekyll-sass-converter just had 3.0.0 release. While the API is compatible, the output is slightly different due to compact output style is no longer supported.

Context

https://github.com/jekyll/jekyll-sass-converter/releases/tag/v3.0.0

@ntkme
Copy link
Contributor Author

ntkme commented Dec 21, 2022

@ashmaroli An alternative to this is to lock jekyll-sass-converter to 2.x in the Gemfile so that all tests would run against 2.x. Not sure which way is preferred.

@ashmaroli
Copy link
Member

@ntkme We need to test with sass-converter-3.x because that's the way forward. We should ideally continue testing with sass-converter-2.x to check backwards-compatibility.

But that aside, why does gem "sass-embedded" require Rubygems v3.2.x on Linux platforms??

@ntkme
Copy link
Contributor Author

ntkme commented Dec 22, 2022

But that aside, why does gem "sass-embedded" require Rubygems v3.2.x on Linux platforms??

rubygems/rubygems#5852
rubygems/rubygems#5889
rubygems/rubygems#5852 (comment)

See links above. This only applies to the “native gems” on Linux platform. Users on older rubygems can still install (and it should default to) platform ruby gem which does not have this limitation. - As far as I know that only case this cause problem is when someone has a lockfile generated with higher version of bundler/rubygems (locked to native gem) and later attempts to install the lockfile on lower version of bundler/rubygems. This issue is solvable by either generate the lockfile from older version of bundler/rubygems, or remove the lockfile, or just make sure the development version and deployment version of ruby/rubygems is the same.

@ashmaroli
Copy link
Member

Users on older rubygems can still install (and it should default to) platform ruby gem which does not have this limitation.

@ntkme, the following is the build log of a Netlify build of our docs site, on this branch:
(There is no prior-generated Gemfile.lock).

4:03:59 AM: Installing gem bundle
4:03:59 AM: [DEPRECATED] The `--path` flag is deprecated because it relies on being remembered across bundler invocations, which bundler will no longer do in future versions. Instead please use `bundle config set path '/opt/build/cache/bundle'`, and stop using this flag
4:03:59 AM: [DEPRECATED] The --binstubs option will be removed in favor of `bundle binstubs`
4:03:59 AM: The dependency tzinfo-data (>= 0) will be unused by any of the platforms Bundler is installing for. Bundler is installing for ruby but the dependency is only for java, x86-mswin32, x86-mingw32, x64-mingw32. To add those platforms to the bundle, run `bundle lock --add-platform java x86-mswin32 x86-mingw32 x64-mingw32`.
4:04:01 AM: Fetching gem metadata from https://rubygems.org/..........
4:04:01 AM: Fetching gem metadata from [https://rubygems.org/.](https://rubygems.org/)
4:04:01 AM: Resolving dependencies....
4:04:02 AM: sass-embedded-1.57.1-x86_64-linux-gnu requires rubygems version >= 3.3.22, which
4:04:02 AM: is incompatible with the current version, 3.1.6
4:04:02 AM: Error during gem install
4:04:02 AM: Build was terminated: Build script returned non-zero exit code: 1
4:04:02 AM: Failing build: Failed to build site
4:04:02 AM: Failed during stage 'building site': Build script returned non-zero exit code: 1 (https://ntl.fyi/exit-code-1)
4:04:02 AM: Finished processing build request in 9.001377524s

@ntkme
Copy link
Contributor Author

ntkme commented Dec 22, 2022

@ashmaroli Yes, I just noticed that it is an issue for ruby 2.7 which by default comes with rubygems 3.1.6. I never noticed it before because ruby/setup-ruby would silently upgrade rubygems when it is too old so I have never seen it fail in GitHub Actions for ruby 2.7.

@ntkme
Copy link
Contributor Author

ntkme commented Dec 22, 2022

So the minimum version of rubygems that works is 3.2.3: https://blog.rubygems.org/2020/12/22/3.2.3-released.html

RubyGems 3.2.3 fixes a long standing bug in the gem client.

gem install now doesn’t try to forcefully install the latest version of the target gem if the current ruby or rubygems version doesn’t meet its required_ruby_version or required_rubygems_version requirements, respectively. Instead, it will try to install the newest version that supports those.

All ruby 3.x comes with rubygems >=3.2.3, so this is only an issue for ruby 2.x. Upgrade to 3.x is probably the simplest solution or user will have to update rubygems.

@ashmaroli
Copy link
Member

Okay @ntkme. Since we have zeroed onto the lowest Ruby version everything works as expected, I recommend updating the sass-embedded README about this.

@ashmaroli
Copy link
Member

@ntkme Please revert the latest commit. We do not wish to enforce Ruby 3.x on contributors just yet. Netlify is configured separately from this repo.

@ntkme
Copy link
Contributor Author

ntkme commented Dec 22, 2022

@ntkme Reverted. I was trying to see if that will fix the netlify build.
https://docs.netlify.com/configure-builds/manage-dependencies/#ruby

Looks like the option here is to set RUBY_VERSION environment variable for netlify, but not sure where is that defined.

@ashmaroli
Copy link
Member

ashmaroli commented Dec 22, 2022

The .ruby-version file is still present in the HEAD branch. Revert wasn't successful.

The environment variable is set in the Netlify dashboard. I have changed it there to Ruby 3.1.2.

@ashmaroli
Copy link
Member

Thanks @ntkme
@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit 572c86e into jekyll:master Dec 22, 2022
jekyllbot added a commit that referenced this pull request Dec 22, 2022
github-actions bot pushed a commit that referenced this pull request Dec 22, 2022
なつき: Update sass related tests for jekyll-sass-converter 3.x (#9223)

Merge pull request 9223
@ntkme ntkme deleted the fix-sass-test branch December 22, 2022 15:40
ashmaroli pushed a commit to ashmaroli/jekyll that referenced this pull request Jan 15, 2023
ashmaroli pushed a commit to ashmaroli/jekyll that referenced this pull request Jan 15, 2023
Update sass related tests for jekyll-sass-converter 3.x
This backports 572c86e to 4.3-stable
ashmaroli pushed a commit that referenced this pull request Jan 15, 2023
Update sass related tests for jekyll-sass-converter 3.x
This backports 572c86e to 4.3-stable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants