run tests with bundler since that is what our users run #1317
Conversation
This file contains 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
|
This seems to reliably cause the Ruby 2.1 integration tests to fail. |
|
test takes 43s locally ... that seems a bit crazy ... will take a look why ... |
grosser
force-pushed
the
grosser/bundle
branch
3 times, most recently
from
b72251b
to
f114526
Compare
|
runs fast locally now .... still times out on travis :( |
|
reproduction steps with bundler 1.15.0+ |
|
idk what's going on in travis, but it's cancelling all builds ... |
|
I guess it cancels all builds when 1 fails ... which won't work here since we have undocumented know failures ... |
|
found the ruby 2.1 bug ... the Gemfile had no gemspec directive :D |
|
green baby :D |
|
only issue left is now the handling of non-standard Gemfile paths ... I"ll address that in another PR |
nateberkopec
reviewed
Jun 7, 2017
lib/puma/launcher.rb
Outdated
| if defined?(Bundler) | ||
| env = Bundler::ORIGINAL_ENV | ||
| # add -rbundler/setup so we load from Gemfile when restarting | ||
| env["RUBYOPT"] = (env["RUBYOPT"].to_s.split(" ") << "-rbundler/setup").uniq.compact.join(" ") |
There was a problem hiding this comment.
Hmmm... are there scenarios where people's Rubyopt will get mangled by this?
There was a problem hiding this comment.
This still makes me nervous. Why aren't we just appending this?
|
Don't know how ... Maybe could duplicate the required but that would not
matter ... maybe if you load puma from a gem and then bundlerBut after ...
But would not be defined at this point then
…On Jun 7, 2017 10:32 AM, "Nate Berkopec" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In lib/puma/launcher.rb
<#1317 (comment)>:
> @@ -163,7 +163,15 @@ def phased_restart
# Run the server. This blocks until the server is stopped
def run
- previous_env = (defined?(Bundler) ? Bundler::ORIGINAL_ENV : ENV.to_h)
+ previous_env =
+ if defined?(Bundler)
+ env = Bundler::ORIGINAL_ENV
+ # add -rbundler/setup so we load from Gemfile when restarting
+ env["RUBYOPT"] = (env["RUBYOPT"].to_s.split(" ") << "-rbundler/setup").uniq.compact.join(" ")
Hmmm... are there scenarios where people's Rubyopt will get mangled by
this?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1317 (review)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAAsZ9kZJziZd4O_7nX4Ah-tMfSDkwVLks5sBt6ZgaJpZM4NvG7z>
.
|
|
@nateberkopec good to go ? |
|
I'm fine with the test changes but mangling RUBYOPT just seems like a recipe for disaster. |
|
isn't just appending more dangerous ... could lead to duplicates
this way it's added unless it's there ... could rewrite to be more ifs and
less split/push/join style ... yeah the uniq could do something weird if
there are multiple -r in there ...
…On Thu, Jun 8, 2017 at 6:48 PM, Nate Berkopec ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In lib/puma/launcher.rb
<#1317 (comment)>:
> @@ -163,7 +163,15 @@ def phased_restart
# Run the server. This blocks until the server is stopped
def run
- previous_env = (defined?(Bundler) ? Bundler::ORIGINAL_ENV : ENV.to_h)
+ previous_env =
+ if defined?(Bundler)
+ env = Bundler::ORIGINAL_ENV
+ # add -rbundler/setup so we load from Gemfile when restarting
+ env["RUBYOPT"] = (env["RUBYOPT"].to_s.split(" ") << "-rbundler/setup").uniq.compact.join(" ")
This still makes me nervous. Why aren't we just appending this?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1317 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAAsZxrbqFtDj7wFyUTo9Zjm9y3yBMnyks5sCKSBgaJpZM4NvG7z>
.
|
|
updated to a safer approach
…On Thu, Jun 8, 2017 at 7:48 PM, Michael Grosser ***@***.***> wrote:
isn't just appending more dangerous ... could lead to duplicates
this way it's added unless it's there ... could rewrite to be more ifs and
less split/push/join style ... yeah the uniq could do something weird if
there are multiple -r in there ...
On Thu, Jun 8, 2017 at 6:48 PM, Nate Berkopec ***@***.***>
wrote:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In lib/puma/launcher.rb
> <#1317 (comment)>:
>
> > @@ -163,7 +163,15 @@ def phased_restart
>
> # Run the server. This blocks until the server is stopped
> def run
> - previous_env = (defined?(Bundler) ? Bundler::ORIGINAL_ENV : ENV.to_h)
> + previous_env =
> + if defined?(Bundler)
> + env = Bundler::ORIGINAL_ENV
> + # add -rbundler/setup so we load from Gemfile when restarting
> + env["RUBYOPT"] = (env["RUBYOPT"].to_s.split(" ") << "-rbundler/setup").uniq.compact.join(" ")
>
> This still makes me nervous. Why aren't we just appending this?
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#1317 (comment)>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/AAAsZxrbqFtDj7wFyUTo9Zjm9y3yBMnyks5sCKSBgaJpZM4NvG7z>
> .
>
|
|
Much better, thx. |
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.
@nateberkopec would have caught #1308 early
does not change a lot since we previously ran the test with
bundle execand so already had all the load-paths etc setup with bundler