search

RefugeRestrooms / refugerestrooms

REFUGE restrooms indexes and maps safe restroom locations for trans, intersex, and gender nonconforming individuals.
http://www.refugerestrooms.org
GNU Affero General Public License v3.0
892 stars 262 forks source link

Fixes Apple M1 Issue w/ Docker #672

Closed etagwerker closed 2 years ago

etagwerker commented 2 years ago

Context

Summary of Changes

Checklist

TODO

Thank you!

DeeDeeG commented 2 years ago

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. :+1:

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 commented 2 years ago

@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 commented 2 years ago

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.