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

shouldn't the opening example include Sinatra::Application.run! ? #1507

Closed
yoLotus opened this issue Dec 17, 2018 · 7 comments
Closed

shouldn't the opening example include Sinatra::Application.run! ? #1507

yoLotus opened this issue Dec 17, 2018 · 7 comments
Labels

Comments

@yoLotus
Copy link

yoLotus commented Dec 17, 2018

Hello,

I tried to run the opening example as written in the first lines of the README but when running ruby myapp.rb it silently exit without listening on port 4567.

I needed to add Sinatra::Application.run! to make it worked:

require 'bundler/inline'
gemfile do
  source 'https://rubygems.org'
  gem 'sinatra'
end

get '/' do
  'Hello world!'
end
Sinatra::Application.run!

Did I miss something?

@namusyaka
Copy link
Member

@yoLotus Thanks for your report. That is good point.
At first, we haven't expected to use require 'sinatra' combined with require 'bundler/inline'
Please take a look at:

at_exit { Application.run! if $!.nil? && Application.run? }

In your case, Application.run? always returns false because of:

set :run, Proc.new { File.expand_path($0) == File.expand_path(app_file) }

app_file indicates actual caller which means bundler/inline.rb so run? always returns false. This is root cause.

In order to avoid the issue quickly, you can do it by setting app_file manually:

require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'
  gem 'sinatra', require: 'sinatra'
end

set :app_file, $0

get '/' do
  'Hello world!'
end

However, this is not understandable. We need to take care the case.

@yoLotus
Copy link
Author

yoLotus commented Dec 23, 2018

Hello @namusyaka , thanks for your detailed answer.

I didn't know about the :app_file setting. I tried your example and It worked perfectly.

I usually use bundler/inline script to test gems or quickly write small scripts with ruby, I don't know if it is a common way to do things but If you think it is relevant to add your example in the documentation, I can handle it in a PR. Where should I add it ?

@namusyaka namusyaka added the bug label Dec 24, 2018
@namusyaka
Copy link
Member

@yoLotus Thanks for confirmation.
I'd like to make implementation handle the case rather than documentation. I've just marked this as a bug.

@yoLotus
Copy link
Author

yoLotus commented Dec 25, 2018

Just a side question but I was wondering if it was really necessary to run application.run? in the main.rb here:

at_exit { Application.run! if $!.nil? && Application.run? } 

Only check that $! is nil shouldn't be enough ?

Otherwise when I modify the main.rb like this, it works without to be forced to explicitly set app_file in my script:

module Sinatra
  class Application < Base
    # we assume that the first file that requires 'sinatra' is the
    # app_file. all other path related options are calculated based
    # on this path by default.
    set :app_file, caller_files.first || $0
    set :expanded_caller_files, caller_files.map{ |f| File.expand_path(f) }
    set :run, Proc.new { expanded_caller_files.include?(File.expand_path($0)) }
    # ...
  end
  at_exit { Application.run! if $!.nil? && Application.run? }
end

But I am not sure it's the best way to do...

@namusyaka
Copy link
Member

@yoLotus I have created a patch for fixing the issue. Could you take a look?

namusyaka added a commit that referenced this issue Feb 3, 2019
@yoLotus
Copy link
Author

yoLotus commented Feb 4, 2019

Hello @namusyaka

Just tried your patch by running the example of my first comment of this thread (by removing the Sinatra::Application.run! statement) it works perfectly, and the diff is really minimal.

Don't know if a test should be add, but if you think it is not necessary, everything seems good on my side.

Thanks a lot for the patch!

namusyaka added a commit that referenced this issue Feb 5, 2019
@namusyaka
Copy link
Member

It's worth testing, of course. Thanks for confirming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants