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

Add Ruby 2.6 #240

Merged
merged 2 commits into from
Feb 15, 2019
Merged

Add Ruby 2.6 #240

merged 2 commits into from
Feb 15, 2019

Conversation

tarebyte
Copy link
Contributor

No description provided.

@dennissivia
Copy link

@tarebyte thanks for your contribution. As far as I understood, there was at least one significant security flaw in ruby 2.6.0. Could you update the PR to use 2.6.1 instead?
When that is done and when CI works, we are glad to merge.

@tarebyte
Copy link
Contributor Author

tarebyte commented Feb 8, 2019

Gah sorry @scepticulous for some reason I thought I opened this on my fork 🤦‍♂️ so apologies for the lack of description or follow up.

I can update to the latest ruby 2.6 since I'm already here 😅

@junaruga
Copy link
Contributor

junaruga commented Feb 8, 2019

Bundler latest version requires Ruby >= 2.3.
https://rubygems.org/gems/bundler/versions/2.0.1
Maybe we can update bundler to the latest version conditionally.

Also this error.

https://travis-ci.org/rack-test/rack-test/jobs/478857681
TypeError:
can't dup NilClass

The reason is maybe below code is okay except Ruby 2.3.
But on Ruby 2.3, below code becomes the TypeError.

On Ruby 2.5 irb

irb(main):001:0> nil.dup
=> nil

On Ruby 2.3 irb

irb(main):001:0> nil.dup
TypeError: can't dup NilClass
	from (irb):1:in `dup'
	from (irb):1
	from /usr/local/ruby-2.3.1/bin/irb:11:in `<main>'

@dennissivia
Copy link

@junaruga I wonder if we should fix the fact that we are using env with a value of nil. Maybe by replacing it with a stubbed value. This would als make the tests more realistic.
What do you think?

@junaruga
Copy link
Contributor

junaruga commented Feb 8, 2019

@scepticulous let me think it.
By the way, I think we can run Travis CI as daily cron mode too.
Seeing the history of the build, the last build is 5 month ago.
That's not good :<

@dennissivia
Copy link

@junaruga I fully agree on the cron mode. Can you take care of that?
Regarding the test adjustments. let me know if you opt for it. In that case I could update the PR to add the adjustment in order to fix the nil.dup issue.

@junaruga
Copy link
Contributor

junaruga commented Feb 8, 2019

@scepticulous for the cron mode, adding the notification to freenode server #rack-test channel might be better at least than current situation.
Though I am not sure I can care, I am going to join the irc channel if the channel is created.

There are 2 cases.

https://github.com/socketry/nio4r/blob/master/.travis.yml

notifications:
  irc: "irc.freenode.org#celluloid"

https://github.com/sass/sass/blob/3.5.6/.travis.yml#L32

@junaruga
Copy link
Contributor

@scepticulous

Regarding the test adjustments. let me know if you opt for it. In that case I could update the PR to add the adjustment in order to fix the nil.dup issue.

I found the reason of the issue. This is because of sinatra latest version 2.0.5. This issue does not happen on sinatra 2.0.4. I think we can wait next version of the sinatra or just pin sinatra adding gem 'sinatra', '2.0.4' to Gemfile.

I opened the ticket.
#243

@dennissivia
Copy link

@tarebyte yes, please push the change to 2.6.1 instead of 2.6.0 and we can merge it.
The build failures are already addressed in other issues and PRs, so we would be glad to merge your PR as soon as its 2.6.1

Copy link

@dennissivia dennissivia left a comment

Choose a reason for hiding this comment

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

I am fine with merging. What about you @perlun @junaruga

@junaruga
Copy link
Contributor

@scepticulous yeah, I am okay.

@dennissivia
Copy link

@junaruga can you merge this dispite the failing jenkins check?
I guess we should just merge the fixes that make CI pass again asap so that we are able to activate travis as a requirement again. otherwise we are not able to accept contributions.

@junaruga junaruga merged commit aee2990 into rack:master Feb 15, 2019
@junaruga
Copy link
Contributor

@scepticulous

guess we should just merge the fixes that make CI pass again asap so that we are able to activate travis as a requirement again.

Yeah I agree to make CI pass again asap.
Let's merge another PR too.

@junaruga
Copy link
Contributor

@tarebyte Thanks Mark!

alex-damian-negru pushed a commit to alex-damian-negru/rack-test that referenced this pull request Apr 5, 2021
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.

None yet

3 participants