Start Spring server process in directory where command was called #621
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When a Spring server boots, it maintains its directory for the duration of its life-cycle. This means that if you've changed directories and run a Spring command, the process will start within the context of the original directory. This is problematic in the use case of engines, where an engine command may be run from the context of the host application, and then run again from the engine directory. In this scenario, the process will assume its working directory is the host application instead of the engine. The solution proposed here is to change the current directory to where the Spring command was called from. The directory is taken from ENV['PWD']
adrianna-chang-shopify
force-pushed
the
spring-process-should-use-working-dir
branch
from
1166cef to
7abf456
Compare
|
I don't think this patch is related to the CI failures, so I'm going to merge it. But we should figure out why the CI is red at some point. |
|
Hello 👋 👋! I found that this patch seems actually related to the CI failures. There are other permanent CI failures related to rubies and rails versions that reached their end of life, but if you restrict the matrix to newer ruby and rails, you can see in this PR how it was previously green, but red if including this patch. In particular, the failing tests, for example spring/test/support/acceptance_test.rb Line 111 in a85d32e try to start a rake task in the root of a generated test application, however due to this change, the rake task is started in the root of the spring repository, causing a missing We were also hit by this in I think the case this PR wanted to support might be already supported by setting |
|
A local repro: $ git showcommit a85d32ef3726931b2b81abbb10a6e1c2964e7e32 (HEAD -> master, tag: v2.1.1, upstream/master, upstream/HEAD)
Author: Guillermo Iguaran <guilleiguaran@gmail.com>
Date: Mon Aug 24 16:52:29 2020 -0700
Prepare v2.1.1
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 59febc8..0c504ad 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,8 @@
## Next Release
+## 2.1.1
+
+* Avoid -I rubylibdir with default-gem bundler
* Start server process in directory where command was called
## 2.1.0
diff --git a/lib/spring/version.rb b/lib/spring/version.rb
index 3ff8b1d..b82db2b 100644
--- a/lib/spring/version.rb
+++ b/lib/spring/version.rb
@@ -1,3 +1,3 @@
module Spring
- VERSION = "2.1.0"
+ VERSION = "2.1.1"
end$ bundle exec rake TESTOPTS=--name=AcceptanceTest#test_tells_the_user_that_Spring_is_being_used_when_used_automatically_via_binstubs$ git revert 20d3a5f --no-edit$ bundle exec rake TESTOPTS=--name=AcceptanceTest#test_tells_the_user_that_Spring_is_being_used_when_used_automatically_via_binstubs |
|
You can see the whole list of new failures by comparing the CI logs.
Sorry for the triple post 🙏, I just wanted to give as much information as possible. |
|
Hey @deivid-rodriguez ! Thanks for providing such a detailed writeup ❤️ Does seem like this introduced some failures, I'll dig into them. In regards to
The problem we were trying to solve here was not that Spring couldn't find the application root, but rather that switching between the host app and an engine and running rails commands with Spring enabled got Spring confused about where the current working directory was. I suspect we need an extra conditional somewhere - I'll take a look. @tenderlove if the fix for this is more obvious to you, let me know 😄 |
|
Hei, thanks! I think I might've figured out a patch to get your use case working. I did the following:
diff --git a/lib/spring/commands/rails.rb b/lib/spring/commands/rails.rb
index f62fe6b..00c683f 100644
--- a/lib/spring/commands/rails.rb
+++ b/lib/spring/commands/rails.rb
@@ -3,7 +3,7 @@ module Spring
class Rails
def call
ARGV.unshift command_name
- load Dir.glob(::Rails.root.join("{bin,script}/rails")).first
+ load Dir.glob(Pathname.new(Dir.pwd).join("{bin,script}/rails")).first || Dir.glob(::Rails.root.join("{bin,script}/rails")).first
end
def descriptionWith these steps, things seem to work as expected, and both test runs work fine and use spring. |
|
Oh, actually, my patch fixes the second case, but breaks your first usage:
😞 |
|
Well, actually, without any changes, |
|
Hey @deivid-rodriguez -- so unfortunately haven't had much success (or time haha) digging into this. This is not a blocker for our setup (we were testing out a monolithic application divided into engines, and so this patch was to get spring operable in the engines). Let's just get this reverted, and my team can reinvestigate at a later point when it becomes higher priority. Thanks again for the writeup, that'll help us out and make sure we don't accidentally cause another regression! |
|
That's very nice of you @adrianna-chang-shopify ❤️. May I create a PR to revert this PR, then? |
|
I went ahead and created #629 👍 |
entcheva
pushed a commit
to thoughtbot/factory_bot_rails
that referenced
this pull request
Jan 12, 2021
Changes introduced in spring 2.1.1 (rails/spring#621) are breaking some tests. That change was reverted in rails/spring#629, but hasn't been released yet. Until #629 is released, this PR skips Spring version 2.1.1. Co-authored-by: Daniel J. Colson <daniel.colson@hey.com>
entcheva
pushed a commit
to thoughtbot/factory_bot_rails
that referenced
this pull request
Jan 12, 2021
* Skip Spring version 2.1.1 Changes introduced in spring 2.1.1 (rails/spring#621) are breaking some tests. That change was reverted in rails/spring#629, but hasn't been released yet. Until #629 is released, this PR skips Spring version 2.1.1. * Bump to latest standard to match CI We are using the latest standard on CI. This bumps the version in the dev Gemfile to match, and fixes one violation. Co-authored-by: Daniel J. Colson <daniel.colson@hey.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When a Spring server boots, it maintains its directory
for the duration of its life-cycle. This means that if you've changed
directories and run a Spring command, the process will start within the
context of the original directory.
This is problematic in the use case of engines, where an engine command
may be run from the context of the host application, and then run again
from the engine directory. In this scenario, the process will assume its
working directory is the host application instead of the engine.
The solution proposed here is to change the current directory to where
the Spring command was called from. The directory is taken from
ENV['PWD']STEPS TO REPRODUCE:
bin/rails test engines/fooin main application => correctly runs theengines/footest suitecd engines/foo; bin/rails test=> runs the main application tests, andDir.pwdin the rails binstubs is the main application