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

Update thin to 1.8.0 to fix bundle install error with xcode 12 on MacOS #40701

Merged
merged 2 commits into from Nov 28, 2020

Conversation

dputtick
Copy link
Contributor

Summary

When I attempt to bundle install a fresh set of Rails dependencies, thin (a dependency of blade) fails to build with native extensions. It looks like I'm hitting the issue described in macournoyer/thin#372. The root cause seems to be a change in how Clang 12 treats a specific compiler warning. Updating thin to 1.8.0 fixes the issue, thanks to this change. The 1.7.2 to 1.8.0 diff is non-trivial, but the vast majority of the changes are to specs.

One thing I am a bit confused about: the blade dependency is listed under ActionCable in the Gemfile, but from grepping around it's only used by ActionView. Am I right that it's only used in the ActionView tests? I'm not super familiar with the Rails codebase. I did see the assets:compile ActionView rake task, but since blade isn't included with the packaged gem, it seems like any non-development code that tried to use it would fail.

Other Information

Worth noting if anybody else runs into this issue that there's a temporary fix described in the PR to thin which worked for me. Would nice to get a vanilla bundle install working, though!

@kaspth
Copy link
Contributor

kaspth commented Nov 26, 2020

Nice! I just hit this today.

I haven't looked at the blade setup in a while, but it's possible that it was replaced (eith rollup?) and the blade dependency was never removed.

@dputtick
Copy link
Contributor Author

@kaspth yeah, it looks like blade was added in 2016 to help build the JS bits of ActionCable, but was replaced with Rollup here #34440. It seems ActionView is still using blade to build rails-ujs for pre Rails 6 backport releases, maybe?

- blade and sprockets-export are only used by ActionView
- blade-sauce_labs_plugin is no longer used, removed in rails#34440
@dputtick
Copy link
Contributor Author

@kaspth I moved the dependencies in the Gemfile for clarity and got rid of blade-sauce_labs_plugin since it wasn't being used anymore. Are you up for reviewing this?

@kaspth
Copy link
Contributor

kaspth commented Nov 27, 2020

Are you up for reviewing this?

Yeah, now that it's no longer 1am my time 😂

It seems ActionView is still using blade to build rails-ujs for pre Rails 6 backport releases, maybe?

Rails < 6 won't receive backport fixes anymore with Rails 6.1 incoming and even if so the builds would happen from a separate release branch (like 5-2-stable). What happens if we just remove it?

@dputtick
Copy link
Contributor Author

Yeah, now that it's no longer 1am my time

@kaspth 😂 oops

After digging a bit more, I was wrong, it turns out blade is still being used for building rails-ujs on Rails 6. So, I think the best option for now is probably to merge this as-is.

@kaspth kaspth merged commit 3524dd9 into rails:master Nov 28, 2020
@kaspth
Copy link
Contributor

kaspth commented Nov 28, 2020

It's past 1am again but what the hell 😂 Thanks for the help!

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