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

Invalid theme folder: _sass when using Github Pages with remote_theme #7630

Closed
cetinajero opened this issue Apr 24, 2019 · 15 comments · Fixed by grupopv/jekyll#1 or #7679
Closed

Invalid theme folder: _sass when using Github Pages with remote_theme #7630

cetinajero opened this issue Apr 24, 2019 · 15 comments · Fixed by grupopv/jekyll#1 or #7679
Labels
bug 🐛 frozen-due-to-age has-pull-request Somebody suggested a solution to fix this issue v3.8 issue Introduced in v3.8. Downgrading to v3.7 resolves issue

Comments

@cetinajero
Copy link
Contributor

cetinajero commented Apr 24, 2019

My Environment

Software Version(s)
Operating System tested on macOS 10.14 and Ubuntu Linux thru circleci/ruby:2.4.1
jekyll Latest (v3.8.5)
github-pages Latest (v198, just released yesterday)
Jekyll-remote-theme Latest (v0.3.1)

Expected Behaviour

A normal build without the warning: Invalid theme folder: _sass.

Current Behavior

Hi, we have a Jekyll theme build for our own Jekyll sites on a separate repo accessed thru jekyll-remote-theme and all have been working just fine until yesterday that we upgraded from v3.7.4 to v3.8.5 (based on the Github pages-gem v198 release).

When using Jekyll v3.8.x, there is a warning showing us: Invalid theme folder: _sass, this warning goes away if Jekyll is downgraded to v3.7.4.

The warning isn't affecting us because the /_sass folder does exists on the Jekyll theme repo and all SCSS files are precompiled just fine. We think that this warning is telling us that the /_sass folder doesn't exist on the main repo but that it's expected and it shouldn't produce any warning.

Screen Shot 2019-04-24 at 12 15 56 PM

What do you suggest?

@cetinajero
Copy link
Contributor Author

Further reading...

It seems that the warning message was introduced in #5195 to offer useful debug information when needed but, in this case... it is throwing false positives when using benbalter/jekyll-remote-theme plugin.

@ashmaroli ashmaroli added the v3.8 issue Introduced in v3.8. Downgrading to v3.7 resolves issue label Apr 25, 2019
@moritzschaefer
Copy link

@cetinajero So that means this message does not reflect an error?

@ashmaroli
Copy link
Member

So that means this message does not reflect an error?

@moritzschaefer No. It's just a warning that the theme you're using doesn't have _sass directory at the root of your current theme.
Warnings are in yellow..
Errors are in red..

@DirtyF DirtyF closed this as completed May 19, 2019
@ashmaroli ashmaroli reopened this May 19, 2019
@ashmaroli
Copy link
Member

@DirtyF Invalid theme folder: _sass is not exactly a user-friendly warning. We could be a bit more verbose..

@ellemenno
Copy link

@ashmaroli Than you, I agree; the warning is confusing. It implies there is something that needs correcting.

I've just encountered this after upgrading to jekyll 3.8 (via github-pages 198), for a theme that does not have a _sass folder.

Why is this a warning? What consequence is there for not including a _sass folder in a theme?

@cetinajero
Copy link
Contributor Author

Warnings are in yellow..
Errors are in red..

Exactly @moritzschaefer, isn't something to worry about too much.

It's just a warning that the theme you're using doesn't have _sass directory at the root of your current theme.

@ashmaroli, in my case there is a _sass directory at the root of my remote_theme, you can see it here.

My theory is that the warning is showing before the jekyll-remote-theme plugin has fetch the contents of the remote theme and, therefore... the _sass directory doesn't exist just yet.

I have added a logger to the realpath_for function like so:

    def realpath_for(folder)
      Jekyll.logger.warn "realpath_for:", folder
      File.realpath(Jekyll.sanitized_path(root, folder.to_s))
    rescue Errno::ENOENT, Errno::EACCES, Errno::ELOOP
      Jekyll.logger.warn "Invalid theme folder:", folder
      nil
    end

And this is the output that I get:

      Generating... 
             Theme: cetinajero/jekyll-theme-marketing
      Theme source: /private/var/folders/h1/95pfc94s5plcr_sjg6ypr64c0000gn/T/jekyll-remote-theme-20190520-22965-1jzho3p
      realpath_for: _sass
Invalid theme folder: _sass
      Remote Theme: Using theme cetinajero/jekyll-theme-marketing
      realpath_for: _sass
      realpath_for: _includes
      realpath_for: _layouts
      realpath_for: assets
                    done in 1.754 seconds.

Please note that the realpath_for: _sass is logged twice:

  • The first one goes to the rescue (Invalid theme folder: _sass) because the _sass directory doesn't exist just yet.

  • The second one (after the "Remote Theme: Using theme cetinajero/jekyll-theme-marketing" message) doesn't fails because the contents of the remote_theme has been fetched.

At least... that's my theory haha...

Will I be right @ashmaroli?

@cetinajero
Copy link
Contributor Author

What consequence is there for not including a _sass folder in a theme?

Not much really, you can use plain CSS and all be good, but... it's better to write all your style using SASS partials @ellemenno.

aetter added a commit to opendistro/for-elasticsearch-docs that referenced this issue May 20, 2019
…tion

Note that local builds now throw invalid warnings as noted in: jekyll/jekyll#7630
ellemenno added a commit to ellemenno/programming-pages that referenced this issue May 21, 2019
@miklb
Copy link
Contributor

miklb commented May 21, 2019

Warnings are in yellow..
Errors are in red..

I consider myself an experienced user and I wouldn't have caught the color of the message meaning "warning". I'm not sure that's the most accessible way to convey the importance, or lack thereof.

What I do see is invalid theme folder
I wouldn't have an issue with it staying a warning, but maybe changing the message to something more inline with the issue. Theme doesn't use _sass

it's better to write all your style using SASS partials

I think we're at a time and place that Jekyll shouldn't force Sass on development. I'm using PostCSS for partials among other optimizations. I'm not suggesting dropping support, just not make assumption it's the only way to process styling files.

@ashmaroli
Copy link
Member

Will I be right @ashmaroli?

@cetinajero Looks like you're right indeed. Good job with the investigation.

but maybe changing the message to something more inline with the issue. Theme doesn't use _sass

@miklb I agree with you as well. The logged message isn't entirely accurate, (and based on comments in this thread, the message can also be misleading in a certain situation.. 🤷‍♂️ )

@cetinajero
Copy link
Contributor Author

cetinajero commented May 24, 2019

Looks like you're right indeed. Good job with the investigation.

Thanks @ashmaroli! I really don't know how to solve it nor why the remote_theme is triggered before and after the fetch, but.. if you give me a North... I could try to put together a PR for review.

@ashmaroli
Copy link
Member

ashmaroli commented May 24, 2019

@cetinajero We won't be able to "fix" jekyll-remote-theme here.

Regarding the dubious log message from Jekyll, we can adapt from another method of the same class:

@root ||= File.realpath(gemspec.full_gem_path)
rescue Errno::ENOENT, Errno::EACCES, Errno::ELOOP
raise "Path #{gemspec.full_gem_path} does not exist, is not accessible "\
"or includes a symbolic link loop"
end

@cetinajero
Copy link
Contributor Author

Hmmm... I was thinking something more complex like checking if the entire root directory was empty... then skip the warning and wait for the second check, but... adapting the message seems fairly easy and doable.

Even with the new message... the warning can be misleading in certain situations and it is present in 3.8.x and 4.0.0.pre.alpha1... can we not only rephrase it but also get rid of the annoying yellow message? (warn => debug)?

Like this two logs on the same class:

Jekyll.logger.debug "Theme:", name
Jekyll.logger.debug "Theme source:", root

@ashmaroli
Copy link
Member

then skip the warning and wait for the second check

That won't be necessary once jekyll-remote-theme has been patched as well..

but also get rid of the annoying yellow message?

Perhaps not in the same PR..

The warning was placed there as an aid for theme developers that their gem doesn't include a certain directory at the gem's root. The "complex" solution would be to log a warning / debug msg, only if the theme wishes to publish sass partials but hasn't been bundled into the gem.

@cetinajero
Copy link
Contributor Author

cetinajero commented May 24, 2019

but also get rid of the annoying yellow message?

When I said "get rid of the annoying message" I was only suggesting lower its level to debug.

The "complex" solution would be to log a warning / debug msg, only if the theme wishes to publish sass partials but hasn't been bundled into the gem.

Even with this solution... the Jekyll theme I'm developing (jekyll-theme-marketing) has sass partials that are bundled into the gem but the Jekyll's Theme.rb class can't see them because they are loaded thru the jekyll-remote-theme plugin, so... this solution doesn't fit my use case.

It seems that rephrasing the msg is the best option, I'll send you the PR in a moment.

cetinajero added a commit to grupopv/jekyll that referenced this issue May 24, 2019
@jekyllbot jekyllbot added the has-pull-request Somebody suggested a solution to fix this issue label May 24, 2019
@cetinajero cetinajero reopened this May 24, 2019
cetinajero added a commit to grupopv/jekyll that referenced this issue May 24, 2019
This change is proposed since the log output it is not actionable for 
the end-user.

Fixes jekyll#7630
cetinajero added a commit to grupopv/jekyll that referenced this issue May 24, 2019
This change is proposed since the log output it is not actionable for
the end-user.

Fixes jekyll#7630
@tian2992
Copy link

tian2992 commented Jun 7, 2019

I'm also affected by this bug.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug 🐛 frozen-due-to-age has-pull-request Somebody suggested a solution to fix this issue v3.8 issue Introduced in v3.8. Downgrading to v3.7 resolves issue
Projects
None yet
8 participants