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

Experimental removal of Rubygems workarounds (main branch) #2380

Merged
merged 2 commits into from
Sep 4, 2020

Conversation

pirj
Copy link
Member

@pirj pirj commented Sep 1, 2020

Due to unstable version resolution and lack of Ruby version constraints on older gemspecs in the Rubygems database, we had to manually declare versions to be used.

This has seemingly been fixed.

Related:
rubygems/rubygems#3463
rubygems/rubygems.org#2474
rubygems/rubygems#2662

Let's see if it works now, and if the issue with IPv6 fallback is still the case.

@pirj pirj self-assigned this Sep 1, 2020
@pirj pirj changed the base branch from master to main September 1, 2020 20:49
@pirj pirj force-pushed the reckless-rubygems-workaround-removal branch from 0d5fee8 to 35a6b21 Compare September 1, 2020 20:50
@pirj pirj changed the title Due to unstable version resolution and lack of Ruby version constraints on older gemspecs in the Rubygems database, we had to manually declare versions to be used. Experimental removal of Rubygems workarounds (main branch) Sep 1, 2020
@benoittgt
Copy link
Member

Ouhao. Super happy if it is working.

@pirj pirj force-pushed the reckless-rubygems-workaround-removal branch 2 times, most recently from d0937af to e09de36 Compare September 2, 2020 20:09
@pirj pirj marked this pull request as ready for review September 2, 2020 20:09
@pirj
Copy link
Member Author

pirj commented Sep 2, 2020

Ran the build several times, it's green apart from a mountable_engine? failure that appears randomly in some builds jobs, but disappears on the second attempt.

3-9-maintenance will probably be even more interesting.
I'll handle the other repos as well after those two.

@JonRowe
Copy link
Member

JonRowe commented Sep 3, 2020

Have we seen the mountable_engine? failure on main yet? It must be related to #2372

Gemfile Outdated Show resolved Hide resolved
Gemfile Outdated
if MAJOR >= 6
gem 'selenium-webdriver', '~> 3.5', require: false
# Rails 6.0 specify '~> 1.4' directly in the adapter.
Copy link
Member

Choose a reason for hiding this comment

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

Do you mean > 1.4? Either way we should let Rails decide what version to use?

Copy link
Member Author

Choose a reason for hiding this comment

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

The comment below makes no sense. When sqlite3 1.5.0 is out, it still matches the Rails' requirement.

But if it happens that sqlite3 decides to jump straight to 2.0, our builds will be broken if we omit this constraint.

I'll fix the comment.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry it was late when I wrote this, what I mean is if the Rails constraint is ~> 1.4 then this has no effect and can be removed. If the rails constraint is > 1.4 then this has an effect but again I don't feel we should constrain this, we need to match Rails here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Point is sqlite3 is an optional Rails dependency, they only call gem when the adapter is set to sqlite in config/database.yml. But still they have a version constraint defined there, which Bundler has no awareness of, since it's outside of the Gemfile.

If we remove it, sqlite3 won't be installed and the build would break.
If we remove the version constraint, the build will break for Rails 6.0 when (eventually, far in the future) sqlite3 2.0 is out.

Copy link
Member

Choose a reason for hiding this comment

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

Right ok, thats not clear from the comment at all.

@pirj pirj force-pushed the reckless-rubygems-workaround-removal branch from efb680e to 5dae54f Compare September 3, 2020 10:02
@pirj pirj requested a review from JonRowe September 3, 2020 10:02
JonRowe
JonRowe previously approved these changes Sep 3, 2020
Gemfile Outdated
if MAJOR >= 6
gem 'selenium-webdriver', '~> 3.5', require: false
# Rails 6.0 specify '~> 1.4' directly in the adapter.
Copy link
Member

Choose a reason for hiding this comment

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

Sorry it was late when I wrote this, what I mean is if the Rails constraint is ~> 1.4 then this has no effect and can be removed. If the rails constraint is > 1.4 then this has an effect but again I don't feel we should constrain this, we need to match Rails here.

Gemfile Outdated Show resolved Hide resolved
Gemfile Outdated Show resolved Hide resolved
@pirj pirj force-pushed the reckless-rubygems-workaround-removal branch from 1dc7b46 to cde18a4 Compare September 3, 2020 12:18
@pirj
Copy link
Member Author

pirj commented Sep 3, 2020

All green, except for mountable_engine? failures. Restarted those jobs - now all green.

With the recent Rubygems server fixes and database backfills, we don't
need most of the manual workarounds to resolve dependency Ruby version
support.
13 is out, no reason to stick to 12
@pirj pirj force-pushed the reckless-rubygems-workaround-removal branch from cde18a4 to 09e465c Compare September 3, 2020 18:58
@JonRowe JonRowe merged commit e5cbfde into main Sep 4, 2020
@JonRowe JonRowe deleted the reckless-rubygems-workaround-removal branch September 4, 2020 08:57
if MAJOR >= 6
gem 'selenium-webdriver', '~> 3.5', require: false
# sqlite3 is an optional, unspecified, dependency and Rails 6.0 only supports `~> 1.4`
gem 'sqlite3', '~> 1.4', platforms: [:ruby]
Copy link
Member Author

@pirj pirj Oct 10, 2020

Choose a reason for hiding this comment

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

example_app_generator/generate_app.rb ends up with

gem 'sqlite3'

in example_app's Gemfile. Why there's no version mismatch, and how the proper version is resolved is beyond my understanding.
I used a different approach for 3-9-maintenance branch, moved this selection to Gemfile-rails-dependencies, and added a statement to strip example_app's Gemfile from gem 'sqlite3'. https://github.com/rspec/rspec-rails/pull/2386/files#diff-8c3955be04b733dbd31e54e50844fb84

PS and it didn't work for Rails 4 https://travis-ci.org/github/rspec/rspec-rails/jobs/734530137

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