Skip to content

print spring client pid on command run #455

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

Merged
merged 3 commits into from
Dec 14, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions bin/spring
Original file line number Diff line number Diff line change
Expand Up @@ -46,4 +46,6 @@ end
lib = File.expand_path("../../lib", __FILE__)
$LOAD_PATH.unshift lib unless $LOAD_PATH.include?(lib) # enable local development
require 'spring/client'
# if the user knows that spring is called, do not show running spring
Spring.quiet = true if $0.include? 'spring'
Spring::Client.run(ARGV)
3 changes: 3 additions & 0 deletions lib/spring/client/run.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
require "spring/commands"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sonalkr132 hey, quick question. I was wondering why you added this line? I'm getting an exception which seems to go away when I remove this line (which I've logged here).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to require spring/command cause it loads the user defined config files (config/spring.rb or ~/.spring.rb).

If user has defined Spring.quite on one of those files then we need different behavior here:

puts "Running via Spring preloader in process #{pid}" unless Spring.quiet

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sonalkr132 @grosser I missed this, but we shouldn't be requiring spring/commands in the client. apart from this issue that @hsume2 reported, loading bundler in the client slows down the execution of spring commands, which is exactly what we want to avoid. The client needs to run as quickly as possible.

For client-side configuration, there is the config/spring_client.rb file, so in theory the user could set Spring.quiet in there (although I don't think this is really something we need to document...)

After fixing this, it would be great if we could somehow prevent a similar bug from being reintroduced in the future (perhaps we can somehow write a test which asserts that bundler is not present in the client? I don't know)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So,Spring.quite option can only be set in spring_client.rb?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the current implementation, I think the answer should be yes. However I think that actually we could write this "Running via Spring preloader" message in Application#serve, by which point spring/commands would be loaded. So I think that may be a better option.

require "rbconfig"
require "socket"
require "bundler"
Expand Down Expand Up @@ -138,6 +139,8 @@ def run_command(client, application)
if pid && !pid.empty?
log "got pid: #{pid}"

puts "Running via Spring preloader in process #{pid}" unless Spring.quiet

forward_signals(pid.to_i)
status = application.read.to_i

Expand Down
4 changes: 3 additions & 1 deletion lib/spring/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

module Spring
class << self
attr_accessor :application_root
attr_accessor :application_root, :quiet

def gemfile
ENV['BUNDLE_GEMFILE'] || "Gemfile"
Expand Down Expand Up @@ -49,4 +49,6 @@ def find_project_root(current_dir)
end
end
end

self.quiet = false
end
26 changes: 25 additions & 1 deletion lib/spring/test/acceptance_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,14 @@ def assert_failure(command, expected_output = nil)
assert_output artifacts, expected_output if expected_output
end

def refute_output_includes(command, not_expected)
artifacts = app.run(*Array(command))
not_expected.each do |stream, output|
assert !artifacts[stream].include?(output),
"expected #{stream} to not include '#{output}'.\n\n#{app.debug(artifacts)}"
end
end

def assert_speedup(ratio = DEFAULT_SPEEDUP)
if ENV['CI']
yield
Expand Down Expand Up @@ -94,6 +102,22 @@ def without_gem(name)
refute app.spring_env.server_running?
end

test "tells the user that spring is being used when used automatically via binstubs" do
assert_success "bin/rails runner ''", stdout: "Running via Spring preloader in process"
assert_success app.spring_test_command, stdout: "Running via Spring preloader in process"
end

test "does not tell the user that spring is being used when the user used spring manually" do
refute_output_includes "spring rails runner ''", stdout: "Running via Spring preloader in process"
refute_output_includes "spring rake test", stdout: "Running via Spring preloader in process"
end

test "does not tell the user that spring is being used when used automatically via binstubs but quiet is enabled" do
File.write("#{app.user_home}/.spring.rb", "Spring.quiet = true")
assert_success "bin/rails runner ''"
refute_output_includes "bin/rails runner ''", stdout: 'Running via Spring preloader in process'
end

test "test changes are picked up" do
assert_speedup do
assert_success app.spring_test_command, stdout: "0 failures"
Expand Down Expand Up @@ -447,7 +471,7 @@ def exec_name
system(#{app.env.inspect}, "bundle install")
end
output = `\#{Rails.root.join('bin/rails')} runner 'require "devise"; puts "done";'`
exit output == "done\n"
exit output.include? "done\n"
RUBY

assert_success [%(bin/rails runner 'load Rails.root.join("script.rb")'), timeout: 60]
Expand Down