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

FFI gem can cause issues for users on Mohave #254

Closed
tombye opened this issue Jul 29, 2021 · 6 comments
Closed

FFI gem can cause issues for users on Mohave #254

tombye opened this issue Jul 29, 2021 · 6 comments

Comments

@tombye
Copy link
Contributor

tombye commented Jul 29, 2021

The ffi gem produces the following error for some users on Mohave (OSX 10.14.6):

dyld: lazy symbol binding failed: Symbol not found: _ffi_prep_closure_loc
  Referenced from: /Users/tombyers/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/ffi-1.15.3/lib/ffi_c.bundle
  Expected in: /usr/lib/libffi.dylib

dyld: Symbol not found: _ffi_prep_closure_loc
  Referenced from: /Users/tombyers/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/ffi-1.15.3/lib/ffi_c.bundle
  Expected in: /usr/lib/libffi.dylib

See ffi/ffi#791 for more details.

I have personally seen in using the GDS way and Notify's tech docs. It has also been an issue on Datagovuk's tech docs.

@timblair suggested it would make sense to pin it in here instead of the downstream projects.

I'm happy to raise a PR to pin it if this seems sensible. Alternatively, I could add a note to the docs if that would be better.

tombye added a commit to alphagov/notifications-tech-docs that referenced this issue Jul 29, 2021
Brings in a bunch of accessibility fixes:

https://github.com/alphagov/tech-docs-gem/blob/master/CHANGELOG.md#240

(And a few patches for bugs found along the way.)

This also bumps the ffi gem which caused this
issue:

ffi/ffi#791

The fix at the moment is just to keep ffi at
1.12 until the issue is resolved so this also adds
that to the Gemfile.

There is an issue on govuk_tech_docs to pin ffi
there instead of downstream apps so we should
revert our pin if this happens:

alphagov/tech-docs-gem#254
@m-green
Copy link
Contributor

m-green commented Jul 30, 2021

The tech writing team has also been experiencing this issue for a while. @PippaClarkGDS I seem to remember the Digital Identity team had a fix, but was it a workaround?

@richardTowers
Copy link
Contributor

For convenience, the options to workaround this from ffi/ffi#791 :

  • Upgrade to macOS 10.15 (Catalina) or higher
  • Tell rubygems not to use the system ffi (gem inst ffi -- --disable-system-libffi)
  • Pin the ffi version back to 1.12.2 in the Gemfile

Also - if anyone's wondering what ffi is, it's the "Foreign Function Interface" library, that allows calls from ruby to native code (usually C). Why do we need it? It's a dependency of sassc (which is the C library we use to compile Sass) and rb-inotify (which middleman uses to detect file changes on linux when it's running in serve mode).


I think pinning ffi in this gem would cause more trouble than it's worth. Because this is a gem (not a project in its own right) we'd have to add an unnecessary direct dependency to ffi in the gemspec, and specify ffi < 1.13. This would force all downstream projects to use ffi 1.12, but if they depend on other gems that specify ffi > 1.12 (or similar) we'd cause a version conflict.

I think it should be up to the individual projects (e.g. gds-way, govuk-developer-docs) to decide whether to pin ffi.

@timblair
Copy link
Member

I don't think we shouldn't be pinning this:

  • This is only an issue on MacOS Mojave (10.14), which is two major releases behind the current release (MacOS 11, Big Sur). Also, going by the history for other MacOS releases, it's reasonable to assume that Mojave will be out of support by the end of 2021 after the release of Monterey.
  • There have already been a number of point and patch releases of ffi since 12.2, and we'll be missing out on improvements (and potential security fixes) from those and other future releases.
  • The root issue at dyld: Symbol not found: _ffi_prep_closure_loc on Macos 10.14 ffi/ffi#791 hasn't moved in over a year, which means the ffi team haven't prioritised this (and given the above, are unlikely to).
  • By pinning here, we're introducing a new direct dependency which might have unintended consequences to projects using this gem.
  • If someone is doing Ruby development on a Mohave machine, it's likely they'll see this issue crop up in other places too, and there are workarounds / solutions to that (see @richardTowers' comment above, or bundle config build.ffi --disable-system-libffi).

I agree with Richard that it should be left up to individual projects on whether to pin (which I also don't think they should, but that's another matter), but it's probably worth adding something to the README in this gem with the suggested fixes if someone does come up against the issue.

@richardTowers
Copy link
Contributor

In an extremely roundabout way we could fix this by replacing the dependency on Sprockets, and using a Middleman "external pipeline" to build the JS / CSS using webpack or something. We already depend on node to install govuk-frontend. In for a penny, in for a pound.

This would remove the need for us to call ffi for anything (it would still need to be installed, through rb-inotify, but I don't think installing it is a problem, it's only when you call it it blows up).

tombye added a commit that referenced this issue Jul 30, 2021
Users of apps consuming this gem and running on OSX Mohave (10.14) can experience an issue with the ffi gem.

This adds a note to the README listing fixes for the issue.

See #254 for related discussion.
@tombye
Copy link
Contributor Author

tombye commented Jul 30, 2021

I've put up a PR to add something to the README.

Worth noting that the Sass project uses Dart-sass rather than sassc now, so we'll have to move away from sassc at some point anyway but I'd guess the work involved is non-trivial. I'm thinking of things like building asset paths, adding SHA hashes to them and how sprockets integrates into the asset pipeline of the consuming app* The actual building of those assets could just be done with NPM scripts.

*but all this may be wrong as I've not been in rails land for years now.

tombye added a commit to alphagov/notifications-tech-docs that referenced this issue Jul 30, 2021
@m-green m-green moved this from Triage to In progress in Technical Documentation Template project board Aug 2, 2021
@m-green
Copy link
Contributor

m-green commented Sep 30, 2021

Documentation has been updated, so closing this issue now. Let me know if I should reopen it.

@m-green m-green closed this as completed Sep 30, 2021
Technical Documentation Template project board automation moved this from In progress to Done Sep 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants