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

Loosen ActionView dep for Rails 5 compatibility. #99

Merged
merged 1 commit into from
Oct 11, 2016

Conversation

danielroseman
Copy link
Contributor

@danielroseman danielroseman commented Oct 9, 2016

Since Travis builds this gem on different Ruby versions, and ActionView 5 won't install on versions before 2.3, we use a conditional expression in the gemspec to ensure it installs the right one.

@@ -32,7 +32,7 @@ library for use in the UK Government Single Domain project}
s.add_dependency "sanitize", "~> 2.1.0"
s.add_dependency 'nokogiri', '~> 1.5'
s.add_dependency 'addressable', '>= 2.3.8', '< 3'
s.add_dependency 'actionview', '~> 4.1'
s.add_dependency 'actionview', RUBY_VERSION >= '2.3' ? '>= 4.1' : '~> 4.1'
Copy link
Member

Choose a reason for hiding this comment

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

Nice workaround 👍 . It might be prudent to add an upper bound of '< 6'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, done.

@boffbowsh
Copy link
Contributor

This will set a dependency on the version of Ruby that is used on rubygems.org to generate the dependencies for Bundler. Also, if the only dependency is deep_symbolize_keys, we should be depending on activesupport not actionview.

Is this change for build and test time, or for production?

@danielroseman
Copy link
Contributor Author

@boffbowsh I'm not sure I understand the distinction - that may be because I don't know what rubygems.org is doing. The point is that I want to be able to use this gem in apps that use Rails 4 or 5. Rails 5 won't even install on earlier Ruby versions, but that shouldn't matter; as I understood it, the dependency graph is calculated when you do bundle install in an app that uses the gem.

(And it turns out that we do use more than deep_symbolize_keys; we include some of the ActionView helpers in the AttachmentPresenter. I'll update the comment.)

@boffbowsh
Copy link
Contributor

In theory, yes, but Bundler precalcs the dependency tree and generates an endpoint like this: https://index.rubygems.org/info/govspeak. Using Ruby to [non-]determine which versions are available means that endpoint will be unreliable.

Could we make this something like ">= 4.1", "< 6" instead?

@danielroseman
Copy link
Contributor Author

That's basically what I had originally, but then Travis failed to build the gem for Ruby 2.1 and 2.2, see https://travis-ci.org/alphagov/govspeak/builds/164866565. I think there are still apps on those versions that use this, so we probably do want to test against them.

@boffbowsh
Copy link
Contributor

I think we get around this in other gems by using different Gemfiles for testing: https://github.com/alphagov/slimmer/tree/master/gemfiles

@danielroseman
Copy link
Contributor Author

@boffbowsh does this look right now?

@boffbowsh
Copy link
Contributor

You'll still need a dependency in the gemspec so that apps know to install actionview. ">= 4.1", "< 6" should be sufficient there

This allows the gem to be included in apps that use Rails 5.

We test against different versions of Ruby, but ActionView 5 does not
build in versions before 2.3. Because the `>=` gem specifier always
finds the latest version, we need to use a different specifier for
earlier Ruby versions, so this configures Travis to use a different
Gemfile for each one.
@danielroseman
Copy link
Contributor Author

Done.

@boffbowsh boffbowsh merged commit 6f8def5 into master Oct 11, 2016
@boffbowsh boffbowsh deleted the rails5-compatibility branch October 11, 2016 14:53
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.

3 participants