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 log output for an invalid theme directory #7679

Merged
merged 6 commits into from Jul 1, 2019

Conversation

Projects
None yet
5 participants
@cetinajero
Copy link
Contributor

commented May 24, 2019

Summary

This fixes an issue since v3.8 where-in a non-actionable message is output to the terminal regarding a directory within current theme (gem / remote) that could either be missing, or be inaccessible, or includes a symlink loop.

  • Update to more verbose terminal output (327bf5b)
  • Change the log-level for this message to debug since it is not actionable for the end-user. (f5a3f40)

The proposed message is:

   Theme exception: The '_sass' directory does not exist at theme's root,
                    is either not accessible or includes a symbolic link loop

Context

Fixes #7630

cc: @ashmaroli

Show resolved Hide resolved lib/jekyll/theme.rb Outdated

@ashmaroli ashmaroli changed the title Update msg when theme subfolder is not available Update log output for an invalid theme directory May 24, 2019

@cetinajero cetinajero force-pushed the cetinajero:fix/theme-sass-message branch from dc9633c to 327bf5b May 24, 2019

@ashmaroli

This comment has been minimized.

Copy link
Member

commented May 24, 2019

I'm not so sure if Jekyll.logger.debug "Warning:", "..." gels well.. However, I'm not able to think of a better alternative either 🤷‍♂

@ashmaroli ashmaroli requested a review from jekyll/build May 24, 2019

@cetinajero

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

I'm not so sure if Jekyll.logger.debug "Warning:", "..." gels well.. However, I'm not able to think of a better alternative either 🤷‍♂

haha yes!, I also thought that. Your call

@cetinajero

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

What about Theme exception: @ashmaroli?, I have gave it some thoughts and that seems the best approach I can think of...

In my bundle exec with debug enabled it can be see it like this:

      Generating... 
             Theme: cetinajero/jekyll-theme-marketing
      Theme source: /private/var/folders/h1/95pfc94s5plcr_sjg6ypr64c0000gn/T/jekyll-remote-theme-20190524-14114-14lbpvi
   Theme exception: The '_sass' directory does not exist at theme's root,
                    is either not accessible or includes a symbolic link loop
      Remote Theme: Using theme cetinajero/jekyll-theme-marketing
      Remote Theme: Downloading https://codeload.github.com/cetinajero/jekyll-theme-marketing/zip/master to /var/folders/h1/95pfc94s5plcr_sjg6ypr64c0000gn/T/jekyll-remote-theme-20190524-14114-1vqogbd.zip
      Remote Theme: Unzipping /var/folders/h1/95pfc94s5plcr_sjg6ypr64c0000gn/T/jekyll-remote-theme-20190524-14114-1vqogbd.zip to /private/var/folders/h1/95pfc94s5plcr_sjg6ypr64c0000gn/T/jekyll-remote-theme-20190524-14114-14lbpvi

Change the log-level for the log output message to debug.
This change is proposed since the log output it is not actionable for
the end-user.

Fixes #7630

@cetinajero cetinajero force-pushed the cetinajero:fix/theme-sass-message branch from edd5d4d to f5a3f40 May 24, 2019

@ashmaroli

This comment has been minimized.

Copy link
Member

commented May 24, 2019

What about Theme exception:

@cetinajero I had thought about Theme Error: but dropped it because it isn't an error every time. If a theme doesn't use Sass partials, then it is not an error / exception. Same applies if a theme doesn't use includes..

@cetinajero

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

So should we name it just a Theme notice: @ashmaroli?

@ashmaroli

This comment has been minimized.

Copy link
Member

commented May 24, 2019

We may have to settle with handling the Exception classes individually:

def realpath_for(folder)
  Jekyll.sanitized_path(root, File.realpath(Jekyll.sanitized_path(root, folder.to_s)))
rescue Errno::ENOENT, Errno::EACCES, Errno::ELOOP => e
  handle_exception(e)
  nil
end

private

def handle_exception(err)
  case err
  when Errno::ENOENT
    nil # no-op
  when Errno::EACCES
    Jekyll.logger.debug "Theme Error:", "Directory '#{folder}' is not accessible."
  when Errno::ELOOP
    Jekyll.logger.debug "Theme Error:", "Directory '#{folder}' includes a symbolic link loop."
  end
end

whatsay?

@cetinajero

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

whatsay?

Great @ashmaroli!

Can I only suggest 3 things?

  • the realpath_for is already a private function, it is defined here:

private
def path_for(folder)
path = realpath_for(folder)
path if path && File.directory?(path)
end
def realpath_for(folder)

  • What about omitting the following code block? It seems rather unnecessary besides explicity demonstrates at source code level that non action is intended. If you prefer to stick to it just let me know.
  when Errno::ENOENT
    nil # no-op

Can I suggest going to Theme error: besides Theme Error:, this follows the same pattern as Theme source:

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

TL;DR:

This is my suggestion:

def realpath_for(folder)
  Jekyll.sanitized_path(root, File.realpath(Jekyll.sanitized_path(root, folder.to_s)))
rescue Errno::ENOENT, Errno::EACCES, Errno::ELOOP => e
  handle_exception(e)
  nil
end

def handle_exception(err)
  case err
  when Errno::EACCES
    Jekyll.logger.debug "Theme error:", "Directory '#{folder}' is not accessible."
  when Errno::ELOOP
    Jekyll.logger.debug "Theme error:", "Directory '#{folder}' includes a symbolic link loop."
  end
end
@ashmaroli

This comment has been minimized.

Copy link
Member

commented May 24, 2019

I kept that "explicit case branch" since that is probably the most commonly occurring scenario out of the three. In such cases, Ruby need not check the other two branches before returning nil.

The alternative is a guard-cause:

return if err.is_a?(Errno::ENOENT)

case err
[...]
@cetinajero

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

return if err.is_a?(Errno::ENOENT)

Neat! 👌

Let me do this changes, do you prefer a whole rebase of this branch?

@ashmaroli

This comment has been minimized.

Copy link
Member

commented May 24, 2019

You need not force-push every commit. Our merge-bot @jekyllbot squashes all commits into one, at the time of merge..

@cetinajero

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

Got it

@cetinajero

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

Do you prefer to stick to the comments?

      # This resolves all symlinks for the theme subfolder and then ensures that the directory
      # remains inside the theme root. This prevents the use of symlinks for theme subfolders to
      # escape the theme root.
      # However, symlinks are allowed to point to other directories within the theme.
@ashmaroli

This comment has been minimized.

Copy link
Member

commented May 24, 2019

Let that comment stay as is. It is outside the scope of this PR..

@ashmaroli

This comment has been minimized.

Copy link
Member

commented May 24, 2019

I think we can now change the log_level to :error since a non-accessible directory or symlink-loop should be brought to the theme-author's notice and rectified..

@cetinajero

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

I think we can now change the log_level to :error since a non-accessible directory or symlink-loop should be brought to the theme-author's notice and rectified..

Yes! haha... you are probably right, and now I'm also realizing that we need to pass the folder variable to handle_exception, check this:

jekyll 3.8.5 | Error:  undefined local variable or method `folder' for #<Jekyll::RemoteTheme::Theme:0x00007fcdbc979a78>

But Travis didn't see that 🤔, we should add a new test case don't you think?

Change the log-level for the log output message to error
Also send the folder to handle_exception
@ashmaroli

This comment has been minimized.

Copy link
Member

commented May 25, 2019

we need to pass the folder variable to handle_exception

Ah yes. My bad..
The overall diff looks fine now.

we should add a new test case

Ideally, we should. But mocking a scenario involving a theme that contains only a certain directory, requires setting up fixtures.
You're welcome to give it a shot by using cues from existing tests. If you're not able to, I shall push a commit after a couple of days..

@cetinajero

This comment has been minimized.

Copy link
Contributor Author

commented May 25, 2019

The overall diff looks fine now.

🎉

Ideally, we should.

Actually I tried throughout the day but didn't come to anything useful at all, the test case is very unique and isolated but maybe on my next contribution I'll accomplish it.

@ashmaroli ashmaroli requested a review from mattr- May 31, 2019

@mattr-

mattr- approved these changes May 31, 2019

@@ -62,11 +62,22 @@ def realpath_for(folder)
# escape the theme root.
# However, symlinks are allowed to point to other directories within the theme.
Jekyll.sanitized_path(root, File.realpath(Jekyll.sanitized_path(root, folder.to_s)))
rescue Errno::ENOENT, Errno::EACCES, Errno::ELOOP
Jekyll.logger.warn "Invalid theme folder:", folder
rescue Errno::ENOENT, Errno::EACCES, Errno::ELOOP => e

This comment has been minimized.

Copy link
@mattr-

mattr- May 31, 2019

Member

I think it makes more sense here to not rescue all three exceptions, but to rescue them individually, so that it's more explicit what's happening here without the extra indirection of the method call

Suggested change
rescue Errno::ENOENT, Errno::EACCES, Errno::ELOOP => e
rescue Errno::ENOENT
# it's ok for things to not exist inside of the theme
end
rescue Errno::EACCES => e
Jekyll.logger.error "Theme error:", "Directory '#{folder}' is not accessible."
end
rescue Errno::ELOOP => e
Jekyll.logger.error "Theme error:", "Directory '#{folder}' includes a symbolic link loop."
end

We can do that now, save it for a later PR, or just let it drop. I'm not going to be that picky about it.

This comment has been minimized.

Copy link
@cetinajero

cetinajero May 31, 2019

Author Contributor

Then every rescue should include a nil so it doesn't return a truthy, right? (see 0278abe)

This comment has been minimized.

Copy link
@ashmaroli

ashmaroli Jun 1, 2019

Member

There's a chance that RuboCop might complain about having too many lines in a method definition or perhaps some other Metric cop flags in.. So let us defer this change for a future PR.

This was referenced Jun 23, 2019

Show resolved Hide resolved lib/jekyll/theme.rb Outdated
@parkr

parkr approved these changes Jul 1, 2019

@ashmaroli

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

@jekyllbot: merge +bug

@jekyllbot jekyllbot merged commit ebe62e8 into jekyll:master Jul 1, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
netlify/jekyllrb/deploy-preview Deploy preview ready!
Details

@jekyllbot jekyllbot added the bug label Jul 1, 2019

jekyllbot added a commit that referenced this pull request Jul 1, 2019

@cetinajero cetinajero deleted the cetinajero:fix/theme-sass-message branch Jul 1, 2019

@ashmaroli

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

Thank you for submitting this patch @cetinajero

@cetinajero

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2019

Thanks you too for your time and review @ashmaroli, I'll be around.

✌️

ashmaroli added a commit to ashmaroli/jekyll that referenced this pull request Jul 1, 2019

Backport jekyll#7679 for v3.8.x
Update log output for an invalid theme directory

ashmaroli added a commit that referenced this pull request Jul 1, 2019

Backport #7679 for v3.8.x (#7734)
Update log output for an invalid theme directory
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.