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

Work with ruby 2.3's --enable-frozen-string-literal - version 2 #1413

Closed

Conversation

larskanis
Copy link
Member

Although bundler is fixed on master in respect to the frozen string literal, no version was released with this fixes, so far. So the travis-ci tests fail before the tests can run, currently.

However when rake test is started without bundler, all tests pass.

@flavorjones
Copy link
Member

@larskanis Thanks for doing this work.

Are you sure that we want to test against 2.3.0 only with the frozen string option set?

I was thinking that perhaps we can refactor the travis matrix in a slightly different way: leave the existing matrix as it is, and then explicitly include two additional builds, which are linux/osx with the frozen string option set.

What do you think?

@flavorjones
Copy link
Member

Also, it looks like the rubygems we're using doesn't support frozen strings yet? Let's hold off on merging until the full toolchain supports it.

@larskanis
Copy link
Member Author

I usually prefer to name the tasks explicitly, that I distribute to someone else... However in this case I don't have a strong opinion. If you don't like it, I'll revert this change.

Are you sure that we want to test against 2.3.0 only with the frozen string option set?

I thought it wouldn't have a valuable benefit, to have two tests on 2.3.0. The frozen-string-literal is an additional restriction, that we should meet (someday). So why should we additionally test for not applying this restriction?

Apart from that, this PR is work in progress. I already fixed one issue in ruby and noticed some remaining issues in bundler. This seems to be enough for nokogiri, but in order to run the bundler tests, a lot of fixes are required on rspec.

@larskanis larskanis changed the title Work with ruby 2.3's --enable-frozen-string-literal - version 2 WIP: Work with ruby 2.3's --enable-frozen-string-literal - version 2 Feb 18, 2016
This inverts the logic for travis-ci tests to make it easier to set
version specific env variables.

Ruby-2.3 doesn't use frozen strings, because rubygems isn't compatible.
@larskanis
Copy link
Member Author

@flavorjones IMHO this is ready for merge now. Frozen strings are enabled for Ruby-2.4 only, because rubygems on Ruby-2.3 isn't compatible.

The test matrix for travis is re-arranged to explicit name the single test runs. At least for me this is much more readable than cross join the rvm and os entries and subtract the excludes from that matrix. In order to enable frozen strings per env variable this matrix would have to be extended by one dimension and all rubyies not supporting frozen strings would have to be added to the exclude list. So I think it's better to simply list all single test runs. Apart from that test runs are equally to the current on the master branch.

@larskanis larskanis changed the title WIP: Work with ruby 2.3's --enable-frozen-string-literal - version 2 Work with ruby 2.3's --enable-frozen-string-literal - version 2 Jan 25, 2017
@flavorjones
Copy link
Member

@larskanis I'm in the process of moving our CI to Concourse, where we'll have more control over the crazy flavors of tests that we want to run; so I'm not worried about the Travis bit.

I can walk you through what the Concourse pipelines look like; but when I merge this I'll likely set up a separate pipeline to test frozen strings.

(You can get a peek at the pipelines at https://ci.nokogiri.org/ and the /concourse/nokogiri.yml.erb file.)

@flavorjones
Copy link
Member

Awesome! Merged, and I added the frozen string RUBYOPT options to the Concourse pipelines in 95684fd.

Thank you!

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.

2 participants