Show nice error message when serve fails

[presidentbeef]

This is a 🙋 feature or enhancement.

I attempted to add tests but couldn't seem to capture a jekyll serve failure without the tests aborting with an error status. Guidance on this would be appreciated.

Summary

If one runs jekyll serve and there is an error, the only message is:

                    ------------------------------------------------
      Jekyll 4.0.0   Please append `--trace` to the `serve` command 
                     for any additional information or backtrace. 
                    ------------------------------------------------

It may seem silly, but it took me a minute to realize anything had gone wrong, as there isn't even a message saying "there was an error".

In my case, this was because I already had something running on port 4000. I suspect this is a common issue.

My suggestion is to at least provide a message stating "Could not start server" if Webrick fails to start.

This PR provides slightly nicer messages for two common cases: address/port in use and access denied for address/port.

However, I am happy to scale it back to a simpler message.

[ashmaroli]

You may refactor this method to be more compact yet more informative.. For example,

def server_error_message(exception, address, port)
  message = case exception
            when Errno::EADDRINUSE
              "Port '#{port}' is already in use."
            when Errno::EACCES
              "Access denied to address '#{address}'."
            else
              exception.to_s  # NOTE: Prefer `e.to_s` over `e.message.to_s`
            end

  # NOTE:  The first argument to Jekyll's logger is best rendered at 20 chars or less.
  Jekyll.logger.error "Server Error:", message
  raise exception
end

[presidentbeef]

Thank you for the suggestions.

It appears that opts["address"] may be empty at the time of the error, so I have only added port information.

Note that server_address pulls the address/port information from the WEBrick instance, which is also not available at the time of the exception.

[ashmaroli]

You could stash the sever address in a local variable before hand and pass it down on error, no?

def start_up_webrick(opts, destination)
  ...
  address = server_address(@server, opts)
  Jekyll.logger.info "Server address:", address
  ...
rescue StandardError => e
  server_error_message(e, opts["port"], address)
end

[presidentbeef]

Oh I don't think I grabbed the right hash key.

However, I don't think the address is that important for the error message, honestly. I don't know if there's a case where you can't bind to an address due to a permissions issue, it's almost always port related. I can adjust but I think it's less confusing to just reference the port number.

[ashmaroli]

okay.. I suggested this because we allow the host and scheme aspects of the address to be configured as well.

[ashmaroli]

@presidentbeef Thank you for submitting this pull request. Please amend your commit to implement the minor changes I've suggested. (Feel free to make other changes as needed..)

[ashmaroli]

@presidentbeef Thank you for amending. This would be complete with some tests included.
(You need not amend again. A fresh commit is fine.)

[DirtyF]

It would be awesome if we could offer to start on another port when port is already in use:

Something is already running at port #{PORT}   
Would you like to run jekyll at another port instead? [Y/n] 

[ashmaroli]

Would you like to run jekyll at another port instead? [Y/n]

Let us defer this particular enhancement for a future PR because of the additional complexity that may delay the current PR from being merged mostly because port is a config option. The related exception will occur after the site has finished building. Offering a choice to change the site's configuration after it has been built needs proper consideration.

[presidentbeef]

@ashmaroli - do you have suggestions for tests? As I noted in my original comment, I was not able to successfully get a failure condition without the tests just exiting, even if I wrapped it in a rescue block, used assert_raises, etc. I was trying to reuse code in test/test_commands_serve.rb.

[ashmaroli]

As I noted in my original comment, I was not able to successfully get a failure condition

Sorry, I missed that part in the OP in spite of it being formatted with an emphasis.
I shall look into adding the tests myself or drop that idea if I can't.. 😉

[nnmuniz]

At least 1 approving review is required by reviewers with write access.

[nnmuniz]

@ashmaroli