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

Override appraisal bundler versions on legacy rubies #2448

Merged
merged 1 commit into from
Dec 1, 2022

Conversation

ivoanjo
Copy link
Member

@ivoanjo ivoanjo commented Nov 30, 2022

What does this PR do?:

This PR fixes #2443 and #2444 by forcing the affected Rubies to run on older versions of bundler.

With this PR I could run bundler exec rake install_appraisal_gemfiles cleanly on my machine.

Motivation:

Make sure the install_appraisal_gemfiles works correctly.

Additional Notes:

(None)

How to test the change?:

Check that the affected rake tasks work correctly.


Fixes #2443
Fixes #2444

**What does this PR do?**:

This PR fixes #2443 and #2444 by forcing the affected Rubies to run
on older versions of bundler.

With this PR I could run `bundler exec rake install_appraisal_gemfiles`
cleanly on my machine.

**Motivation**:

Make sure the `install_appraisal_gemfiles` works correctly.

**Additional Notes**:

(None)

**How to test the change?**:

Check that the affected rake tasks work correctly.
@ivoanjo ivoanjo requested a review from a team November 30, 2022 15:31
@ivoanjo ivoanjo merged commit e3bfe66 into master Dec 1, 2022
@ivoanjo ivoanjo deleted the ivoanjo/override-bundler-versions-on-legacy-rubies branch December 1, 2022 10:24
@github-actions github-actions bot added this to the 1.8.0 milestone Dec 1, 2022
sh "docker-compose run -e APPRAISAL_GROUP --no-deps --rm tracer-#{version} /bin/bash -c " \
"'rm -f Gemfile.lock && gem update bundler && bundle install && bundle exec appraisal update'"
"'rm -f Gemfile.lock && #{bundler_setup} && bundle #{forced_bundler_version} install && bundle exec appraisal #{forced_bundler_version} update'"
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't that run bundle exec appraisal generate as well?

IIRC gemfiles are not regenerated when the files are found to exist.

Copy link
Member

Choose a reason for hiding this comment

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

Additionally, we should ideally run bundle lock --lockfile <lockfile> --add-platform [x86_64-linux,aarch64-linux] when possible (I think it's not supported on 1.17.x)

Copy link
Member Author

Choose a reason for hiding this comment

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

@lloeki I think you're right. Would you be up for opening a PR adding that to both tasks?

Copy link
Member Author

Choose a reason for hiding this comment

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

Additionally, we should ideally run bundle lock --lockfile --add-platform [x86_64-linux,aarch64-linux] when possible (I think it's not supported on 1.17.x)

Actually, @lloeki isn't that blocked on the sorbet changes on the Gemfile?

Copy link
Member Author

Choose a reason for hiding this comment

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

IIRC gemfiles are not regenerated when the files are found to exist.

I've just tried this and I think they are. As an example, I added an extra gem to one of the existing appraisal gemfiles, then ran bundle exec rake install_appraisal_gemfiles and the lockfiles were not changed BUT the gemfile was regenerated and the extra gem was removed.

Copy link
Member

Choose a reason for hiding this comment

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

Yes it is, we need to move forward with @marcotc's PR #2421

@@ -15,22 +15,35 @@ TRACER_VERSIONS = %w[
].freeze
# ADD NEW RUBIES HERE

FORCE_BUNDLER_VERSION = {
'2.3' => '1.17.3', # Some groups require bundler 1.x https://github.com/DataDog/dd-trace-rb/issues/2444
'jruby-9.2.8.0' => '2.3.6', # 2.3.26 seems to be broken https://github.com/DataDog/dd-trace-rb/issues/2443
Copy link
Member

Choose a reason for hiding this comment

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

Surprising, I was able to run with 2.3.26 locally without issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I retried it locally, both with JRUBY_OPTS=--dev as well as without (because the default in our docker-compose is without, but on CI is with that setting) and I could not reproduce it.

Since the workaround seems simple, I'd still leave it in (we've seen it fail, so we know it can fail at least rarely), but if you're not convinced I'm happy to revert it.

Copy link
Member

Choose a reason for hiding this comment

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

No problem with keeping it, it just feels odd so we may not have been down to the bottom of this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, that's fair but... I'm not sure it's worth getting to the bottom of this.

The issue was caused by a NullPointerException inside JRuby. That right there should never happen, and suggests an issue with JRuby.

And this issue is on 9.2.8.0, which is known to be very buggy, which is why where are 13 other releases in the 9.2 series, over the span of 3 years.

So my thinking is -- it's quite an investment to chase this down, and in the end we may learn that it's been fixed by a later version anyway, and we still will need to keep 9.2.8.0 around for now :/

ivoanjo added a commit that referenced this pull request Jan 20, 2023
**What does this PR do?**:

This PR disables testing of JRuby 9.2.8.0 in CI, as well as
appraisal generation for it (as well as removes the existing
appraisals).

**Motivation**:

Maintaining JRuby 9.2.8.0 working has been quite a burden: this version
has many, many, many known issues -- you will notice that there have
been **13** other bugfix releases on top of it; latest JRuby is
9.2.31.0.

Some examples of PRs where this JRuby version caused toil are
 #2565, #2558, #2448, #2412, #1869, etc

We've decided to stop running it in CI for now, and plan to evaluate
if we should remove it entirely from our test harness.

We recommend that all our customers use JRuby 9.2.21.0, and also
recommend evaluating the move to JRuby 9.3, as the JRuby developers have
marked JRuby 9.2 as end-of-life:
<https://twitter.com/jruby/status/1611063274459566106>.

**Additional Notes**:

I didn't have to remove the appraisal gemfiles from the tree, but
since I disabled the tasks that automatically keep them up-to-date,
they wouldn't be maintained anyway.

If/when we do want to revert this change, we should regenerate them using
the usual rake tasks.

**How to test the change?**:

Validate that CI is green and that the JRuby 9.2.8.0 configuration is
not shown there.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants