Skip to content

Fix upgrade/remove with new binstub variations #445

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

Conversation

felixbuenemann
Copy link
Contributor

This is a followup to #444 which adds support for upgrading to the new loader code as well as supporting bin/spring binstub --remove with both single and double quoted variants of the previous loader code, which was not handled in #437.

I've also added an acceptance test for bin/spring binstub --remove --all which tests for the removal of bin/spring and added a mention of the --remove option to the spring client binstub help output.

There are three variations of the new style spring binstub:
* older with double quotes
* newer with single quotes
* newest which does not swallow unrelated load errors

This does not handle removing the old style if/else binstub. To remove
is a two-step upgrade and remove is required.
@rails-bot
Copy link

r? @rafaelfranca

(@rails-bot has picked a reviewer for you, use r? to override)

@felixbuenemann
Copy link
Contributor Author

@rafaelfranca This should probably be assigned to @grosser, because he handled #444.

@@ -48,6 +48,12 @@ class Binstub < Command

OLD_BINSTUB = %{if !Process.respond_to?(:fork) || Gem::Specification.find_all_by_name("spring").empty?}

BINSTUB_VARIATIONS = Regexp.union [
%{begin\n load File.expand_path("../spring", __FILE__)\nrescue LoadError\nend\n},
Copy link
Collaborator

Choose a reason for hiding this comment

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

these 2 lines look pretty similar ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah the quotes ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Double vs. single quotes.

@felixbuenemann
Copy link
Contributor Author

r? @grosser

@rails-bot rails-bot assigned grosser and unassigned rafaelfranca Nov 17, 2015
@grosser
Copy link
Collaborator

grosser commented Nov 17, 2015

great tests! :)

grosser added a commit that referenced this pull request Nov 17, 2015
…nstub-variations

Fix upgrade/remove with new binstub variations
@grosser grosser merged commit 727e4b8 into rails:master Nov 17, 2015
@grosser
Copy link
Collaborator

grosser commented Nov 17, 2015

1.4.3 since it's kind of a fix for the previous ... even though it includes a new feature ... but should be safe ...

@felixbuenemann felixbuenemann deleted the fix-upgrade-remove-new-binstub-variations branch November 17, 2015 03:21
@felixbuenemann
Copy link
Contributor Author

I agree, this is a bug fix release. Thanks for taking the time to review.

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

Successfully merging this pull request may close these issues.

4 participants