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.

Sign up for GitHub

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
Merged

Override appraisal bundler versions on legacy rubies #2448

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 24 additions & 3 deletions tasks/update_appraisal_gemfiles.rake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 :/

}.freeze

desc 'Installs gems based on Appraisals and Gemfile changes, ' \
'accepts list of tracer versions as task argument, defaults to all versions.'
task :install_appraisal_gemfiles do |_task, args|
tracer_version_arg = args.to_a
versions = tracer_version_arg.empty? ? TRACER_VERSIONS : tracer_version_arg

versions.each do |version|
forced_bundler_version = nil
bundler_setup = 'gem update bundler'

if FORCE_BUNDLER_VERSION.include?(version)
forced_bundler_version = "_#{FORCE_BUNDLER_VERSION[version]}_"
bundler_setup = "gem install bundler -v #{FORCE_BUNDLER_VERSION[version]}"
end

sh [
"docker-compose run -e APPRAISAL_GROUP --no-deps --rm tracer-#{version} /bin/bash -c ",
"'rm -f Gemfile.lock && gem update bundler && bundle install && ",
"'rm -f Gemfile.lock && #{bundler_setup} && bundle #{forced_bundler_version} install && ",
# Appraisal runs `bundle check || bundle install` on `appraisal install`. This skips `bundle install` \
# if the `Gemfile.lock` is satisfied by installed gems, even if there are `Gemfile` changes to be processed. \
# Adding the `--without` option forces Appraisal to skip `bundle check` and always run `bundle install`. \
# `--without` has a small side-effect of getting saving in the local bundler env, but we do not persist \
# these changes outside of the current container. \
"bundle exec appraisal install --without force-appraisal-to-always-run-bundle-install'"
"bundle exec appraisal #{forced_bundler_version} install --without force-appraisal-to-always-run-bundle-install'"
].join
end
end
Expand All @@ -42,7 +55,15 @@ task :update_appraisal_gemfiles do |_task, args|
versions = tracer_version_arg.empty? ? TRACER_VERSIONS : tracer_version_arg

versions.each do |version|
forced_bundler_version = nil
bundler_setup = 'gem update bundler'

if FORCE_BUNDLER_VERSION.include?(version)
forced_bundler_version = "_#{FORCE_BUNDLER_VERSION[version]}_"
bundler_setup = "gem install bundler -v #{FORCE_BUNDLER_VERSION[version]}"
end

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

end
end