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

Fixes Apple M1 Issue w/ Docker #672

Merged
merged 2 commits into from Apr 28, 2022

Conversation

etagwerker
Copy link
Contributor

Context

Summary of Changes

  • Set platform arm64 for web container
  • Stop using ActiveSupport::EventedFileUpdateChecker

Checklist

  • Tested Mobile Responsiveness
  • Added Unit Tests
  • CI Passes
  • Deploys to Heroku on test Correctly (Maintainers will handle)
  • Added Documentation (Service and Code when required)

TODO

  • I don't have an Apple Intel MacBook at the moment. It would be great if someone could test my changes with an Apple Intel environment.

Thank you!

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Apr 24, 2022

Looks good, thanks for this.

Noting for the record these are both development-only changes. Should not affect production whatsoever.


Setting the platform: linux/amd64 for the web Docker container seems both correct and a trivial fix for M1 / ARM users, so +1 from me there.

Details (click to expand):

(It is indeed supposed to be/only knowingly tested/used so far as an x64 Linux container. No idea whether or how well it works on an ARM image, but not something worth looking into if we don't have to, IMO. More important things to spend the time on.)

Docs in case this somehow comes up as an issue (which I doubt it would): https://docs.docker.com/compose/compose-file/#platform

platform defines the target platform containers for this service will run on, using the os[/arch[/variant]] syntax. Compose implementation MUST use this attribute when declared to determine which version of the image will be pulled and/or on which platform the service’s build will be performed.


Re config.file_watcher:

-  config.file_watcher = ActiveSupport::EventedFileUpdateChecker
+  # config.file_watcher = ActiveSupport::EventedFileUpdateChecker

Also seems fine to me. 👍

Details...

It looks like Rails provides a different watcher by default, which can evidently work on all platforms if it fixes the issue for you on an Apple M1 Mac via qemu. (Works for me directly on a genuine x64 Linux host, as opposed to through emulation or hypervisor layers, as tend to be required on other platforms.)

For documentation of the default watcher Rails provides here and presumably falls back on in this case, I found:

https://guides.rubyonrails.org/v5.2/configuring.html#rails-general-configuration / https://guides.rubyonrails.org/configuring.html#config-file-watcher

config.file_watcher is the class used to detect file updates in the file system when config.reload_classes_only_on_change is true. Rails ships with ActiveSupport::FileUpdateChecker, the default, and ActiveSupport::EventedFileUpdateChecker (this one depends on the listen gem). Custom classes must conform to the ActiveSupport::FileUpdateChecker API.

Maybe this default watcher is theoretically less optimized for disk access, but I haven't noticed any show-stopping performance issue in brief testing, and I think being able to run the Docker container is more important than speed.


If you don't mind, I'll wait a day or two to give the other maintainers a chance to review as well. Otherwise I plan to merge this in a few days if there are no comments or complaints. Thanks again!

@etagwerker
Copy link
Contributor Author

@DeeDeeG Thanks for the additional details about my change!

Re config.file_watcher

Would you prefer it for me to remove the lines from that file? I know I left it as a comment, but I know that might add noise in the future.

If you don't mind, I'll wait a day or two to give the other maintainers a chance to review as well.

All good, thanks for the prompt review!

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Apr 26, 2022

Hi,

the comment is fine. I think it will be easier to remember this was disabled on purpose, with it commented out.

(We occasionally upgrade the Rails version, and during that process, it suggests merging new versions of these config files. So we have to remember what was changed for a reason, or decide if the new defaults are better, etc.)

If you want to add another comment line, something like this (see below), that would be cool. But I think I will remember why this was disabled, at least.

# Disabled for Apple M1 + Docker compatibility, see https://github.com/RefugeRestrooms/refugerestrooms/pull/672

Cheers.

Copy link
Contributor

@DeeDeeG DeeDeeG left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

@DeeDeeG DeeDeeG merged commit a6e6ef3 into RefugeRestrooms:develop Apr 28, 2022
@DeeDeeG DeeDeeG mentioned this pull request Oct 10, 2022
5 tasks
DeeDeeG added a commit that referenced this pull request Oct 11, 2022
* Fixes Apple M1 Issue w/ Docker (#672)
  - This causes problems in docker+apple m1
      (`ActiveSupport::EventedFileUpdateChecker`
      isn't working on Docker for Apple Silicon (M1) at the moment.)
      More here: evilmartians/terraforming-rails#34
  - Make sure we use amd64 for Mac OS X M1 compatibility

* Update Dependencies for May 2022 (#673)
  - ruby: Upgrade Ruby from 2.7.5 to 2.7.6
  - Gemfile.lock: Update nokogiri from 1.13.3 to 1.13.6
  - Gemfile[.lock]: Update rails (5.2.6.3 --> 5.2.8) and its dependencies
      Also updates a few indirect dependencies.
      Specifically: concurrent-ruby, i18n, loofah, mini_mime, and thor.
  - Gemfile.lock: Update puma from 5.6.2 to 5.6.4
  - yarn.lock: Update minimist from 1.2.5 to 1.2.6
  - yarn.lock: Update node-forge from 1.2.1 to 1.3.1
  - yarn.lock: Update swagger-ui from 4.1.3 to 4.7.0
      Also update some of its dependencies, such as @braintree/sanitize-url.
  - yarn.lock: Update async from 2.6.3 to 2.6.4
  - Revert "yarn.lock: Update swagger-ui from 4.1.3 to 4.7.0"
      It broke when updating, not sure why. Possibly webpack-related???
      Back to swagger-ui 4.1.3.
  - deps: Resolve @braintree/sanitize-url to v6.x
      Was v5.x. (This is a JS dependency, in package.json and yarn.lock.)
  - yarn.lock: Update eventsource from 1.0.7 to 1.1.1
  - Gemfile.lock: Update rack from 2.2.3 to 2.2.3.1

* update to new maps public key (#675)
  - update to new public key
  - upgrade to rails 6
  - add net gems to Gemfile
  - update lock file
  - fix linting errors with upgrade
  - use proper "visible" syntax
  - update API gems
  - change to old css rspec helper

* Update dependencies for early October 2022 (#676)
  - yarn.lock: Update terser
  - yarn.lock: Dedupe dependencies
  - ruby: Bump Ruby from 3.1.0 to 3.1.2
      (Was recently updated from 2.7.6 to 3.1.0 in PR 675)

Co-authored-by: Mikena Wood <mi-wood@users.noreply.github.com>
Co-authored-by: Ernesto Tagwerker <ernesto+github@ombulabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Function not implemented - Failed to initialize inotify (Errno::ENOSYS) Apple M1 linux/amd64 platform
2 participants