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

Fail gracefully if "sass" gem cannot be loaded #6573

Merged
merged 1 commit into from Nov 21, 2017

Conversation

Projects
None yet
4 participants
@ashmaroli
Member

ashmaroli commented Nov 21, 2017

Closes #6423

Print our custom error message and let the user know they need to install "sass" gem instead of showing them the backtrace.

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Nov 21, 2017

Member

@ashmaroli Wow, nice sleuthing 🔍

Member

pathawks commented Nov 21, 2017

@ashmaroli Wow, nice sleuthing 🔍

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Nov 21, 2017

Member

Thank you 😃

Member

ashmaroli commented Nov 21, 2017

Thank you 😃

@DirtyF

DirtyF approved these changes Nov 21, 2017

Good catch!

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Nov 21, 2017

Member

@jekyllbot: merge +bug

Member

pathawks commented Nov 21, 2017

@jekyllbot: merge +bug

@jekyllbot jekyllbot merged commit c2586bb into jekyll:master Nov 21, 2017

3 checks passed

WIP ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jekyllbot jekyllbot added bug fix labels Nov 21, 2017

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Nov 21, 2017

Member

Maybe we should backport this to 3.6.x and cut a new release.

@ashmaroli Any desire to write a short announcement post about fixing this?

Member

pathawks commented Nov 21, 2017

Maybe we should backport this to 3.6.x and cut a new release.

@ashmaroli Any desire to write a short announcement post about fixing this?

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Nov 22, 2017

Member

IMO, this doesn't warrant a separate release. The only change is that Jekyll is going to output the usual friendly-error message telling the user that they don't have sass installed, instead of the backtrace.
Actually, this issue is just an edge-case because, for >90% of users, sass will be installed automatically by bundle install because of it being a second-order runtime dependency.

jekyll < jekyll-sass-converter < sass

Regarding the associated issue ticket, there's every chance that Jekyll on Gentoo build still fails, (I've no idea why Bundler did not include sass in the build nor why it did not install sass if it wasn't installed in the system, to begin with..) I chose to close it with this because, I feel this is all that can be done from our side.

Any lastly, why backport this to just 3.6.x, when the bug can be traced to much older releases..?

Member

ashmaroli commented Nov 22, 2017

IMO, this doesn't warrant a separate release. The only change is that Jekyll is going to output the usual friendly-error message telling the user that they don't have sass installed, instead of the backtrace.
Actually, this issue is just an edge-case because, for >90% of users, sass will be installed automatically by bundle install because of it being a second-order runtime dependency.

jekyll < jekyll-sass-converter < sass

Regarding the associated issue ticket, there's every chance that Jekyll on Gentoo build still fails, (I've no idea why Bundler did not include sass in the build nor why it did not install sass if it wasn't installed in the system, to begin with..) I chose to close it with this because, I feel this is all that can be done from our side.

Any lastly, why backport this to just 3.6.x, when the bug can be traced to much older releases..?

@ashmaroli ashmaroli deleted the ashmaroli:gracefully-require-sass branch Nov 22, 2017

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Nov 22, 2017

Member

Ok.

Member

pathawks commented Nov 22, 2017

Ok.

DirtyF added a commit that referenced this pull request Dec 7, 2017

DirtyF added a commit that referenced this pull request Dec 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment