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

Remove ruby platform check for sorbet dependency #2445

Closed
wants to merge 1 commit into from

Conversation

TonyCTHsu
Copy link
Contributor

@TonyCTHsu TonyCTHsu commented Nov 29, 2022

What does this PR do?

With the ruby platform check, the appraisal lockfile would always end up with difference. However, currently we are using a compatible version that could be installed on x86 or arm64-darwin21 from M1 Mac. So we could remove this conditional

How to test this

With M1 Mac, bundle install && bundle exec rake typecheck

@codecov-commenter
Copy link

Codecov Report

Merging #2445 (66fd91a) into master (970c87a) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2445      +/-   ##
==========================================
- Coverage   98.34%   98.33%   -0.01%     
==========================================
  Files        1101     1101              
  Lines       58977    58974       -3     
==========================================
- Hits        58001    57993       -8     
- Misses        976      981       +5     
Impacted Files Coverage Δ
spec/support/http_helpers.rb 90.90% <0.00%> (-9.10%) ⬇️
spec/support/synchronization_helpers.rb 81.57% <0.00%> (-2.64%) ⬇️
lib/datadog/core/diagnostics/environment_logger.rb 98.42% <0.00%> (-1.58%) ⬇️
...adog/tracing/contrib/sidekiq/server_tracer_spec.rb 99.20% <0.00%> (-0.80%) ⬇️
...ng/contrib/active_support/cache/instrumentation.rb 87.58% <0.00%> (-0.09%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@marcotc
Copy link
Member

marcotc commented Nov 29, 2022

Waiting on sorbet/sorbet#4119 to be addressed. There's already a PR up for it, currently under review.

Copy link
Member

@ivoanjo ivoanjo left a comment

Choose a reason for hiding this comment

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

Waiting on sorbet/sorbet#4119 to be addressed. There's already a PR up for it, currently under review.

Note that upstream seems very slow on this issue, so I don't think we can expect a fix anytime soon 😰

Comment on lines -91 to +92
# Also, there's no support for windows
if RUBY_VERSION >= '2.4.0' && (RUBY_PLATFORM =~ /^x86_64-(darwin|linux)/)
# Also, there's no support for windows, but M1 Mac with `arm64-darwin21` architecture is doing fine
if RUBY_VERSION >= '2.4.0'
Copy link
Member

Choose a reason for hiding this comment

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

Note that there's two changes being made here: enabling it on arm64 for macOS and for Linux. Afaik it doesn't work for Linux /yet/, so I think we should leave that part of the check for now.

Copy link
Member

Choose a reason for hiding this comment

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

but M1 Mac with arm64-darwin21 architecture is doing fin

aarch64-linux is not! This can't be merged as is.

This PR from @marco is the only sane way to do it: #2421

@lloeki
Copy link
Member

lloeki commented Nov 30, 2022

With the ruby platform check, the appraisal lockfile would always end up with difference

Important note: the locks that needs to be commited to work under aarch64-linux MUST NOT have sorbet, as this prevents bundle lock --lockfile=ruby_x.y.z_foo.lock --add-platform aarch64-linux from working.

Ideally the appraisal lockfiles shoud all have PLAFTORMS with both x86_64-linux and aarch64-linux but not ruby.

As I said to @marcotc the other day, conditionals in Gemfiles are bad as they are evaluated on the currently run RUBY_PLATFORM + RUBY_VERSION, which has terrible effects on the descriptiveness of the DSL preventing proper cross platform + cross version usage of bundler.

I have a tentative change locally to address the Gemfile conditionals, making it entirely descriptive but didn't push a PR yet because I have a path issue to deal with when using the appraisal command.

@lloeki
Copy link
Member

lloeki commented Nov 30, 2022

Note that upstream seems very slow on this issue, so I don't think we can expect a fix anytime soon

It's been working on a fork for something like a year. It's clearly just not their priority.

@ivoanjo
Copy link
Member

ivoanjo commented Nov 30, 2022

Important note: the locks that needs to be commited to work under aarch64-linux MUST NOT have sorbet, as this prevents bundle lock --lockfile=ruby_x.y.z_foo.lock --add-platform aarch64-linux from working.

👍 thanks for the reminder. To be honest we don't need sorbet for the appraisal steps, so even without #2421 I think we could fix this by for instance tweaking the condition so that it doesn't trigger when the Gemfile is being evaluated for the appraisals.

But #2421 does seem to be a less hacky way of doing it, so that's probably the way to go.

@TonyCTHsu TonyCTHsu closed this Dec 1, 2022
@GustavoCaso GustavoCaso deleted the tonycthsu/platform-typecheck-condition branch October 11, 2023 11:42
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.

5 participants