-
Notifications
You must be signed in to change notification settings - Fork 87
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
Added Support for Dockerized Apple Silicon Installations - PR 311 Fix #314
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThe changes include the addition of a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Docker as Docker Environment
participant App as Application
User->>Docker: Run Docker with `--platform` flag
Docker->>Docker: Build image using specified PLATFORM
Docker->>App: Deploy application with correct architecture
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
Dockerfile
Outdated
|
||
FROM --platform=linux/${PLATFORM} ${BUILDER_IMAGE} as builder_arm64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand the tricks with builder_amd64
and builder_arm64
😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wasn't very intuitive to make the sure that the platform uses the correct elm binary so I did some research and found that the best way to do it was to use separate build steps, 1 for each platform, and the only use that specific image for the given platform. It took a while to stumble upon this article that helped me create this.
For example, if we use "PLATFORM=amd64", then the builder
image will use builder_amd64
since builder
is aliasing builder_${PLATFORM}
. builder_amd64
will run the single line below it, downloading the elm binary from the official github as opposed to pulling it from the custom image that I created.
So it creates 2 possible images that we would use as builder
, then uses the one that matches the platform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@loicknuchel I am actually unsure of the performance this may have on amd64 builds since it may still try to run the COPY under builder_arm64. I'll run a check on this tomorrow, and if its significantly slower, I may just have it download the binary instead of pulling it from the image.
I didn't go that way initially since I don't love the idea of pulling a raw binary from a non-official repo, but we could do a SHA check and make sure it matches the one generated by that dockerfile I created.
Thanks for the reviews @SamirPS @loicknuchel ! I'll address them this evening and make things that are hard-coded easier to understand / made into a variable where necessary. |
Thank @MrKovar |
…ose.yml to use environment variables for Platform variable. Added Platform variable to .env.example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- .env.example (1 hunks)
- Dockerfile (5 hunks)
- INSTALL.md (1 hunks)
- docker-compose.yml (1 hunks)
Files skipped from review due to trivial changes (1)
- .env.example
Additional comments not posted (8)
docker-compose.yml (2)
14-14
: Dynamic platform specification enhances flexibility.Replacing the hardcoded platform value with
linux/$PLATFORM
allows for more adaptable deployments across different architectures.
18-19
: PassingPLATFORM
as a build argument improves compatibility.The addition of the
args
section ensures that the build process can adapt to different architectures by using the specified platform.Dockerfile (5)
19-19
: Default value forELM_VERSION
enhances consistency.Setting a default value for
ELM_VERSION
reduces the risk of build errors and ensures consistency across builds.
32-32
: Default value forPLATFORM
improves usability.Setting a default value for
PLATFORM
ensures that the build process can proceed smoothly without requiring explicit specification.
34-34
: Platform-specific builder images enhance compatibility.Incorporating the
PLATFORM
variable into theBUILDER_IMAGE
argument allows for platform-specific builds, improving compatibility with different architectures.
40-46
: Platform-specific builder stages improve compatibility.Introducing
builder_amd64
andbuilder_arm64
ensures that the correct Elm binary is used for each platform, enhancing compatibility and performance.
122-122
: Platform-specific final image enhances flexibility.Using the
PLATFORM
variable for the final image stage allows for tailored builds, improving flexibility and usability across different architectures.INSTALL.md (1)
58-66
: Tailored instructions for Apple Silicon/Arm64 users enhance usability.The new section provides specific installation instructions for Apple Silicon/Arm64 users, improving documentation and usability for this user group.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
I would love to have this available to run on my Mac, in case the love I send your way happens to give any added motivation 💚😉 |
Fix for issue 295: #295
When running the Dockerfile on Apple Silicon the container will crash with a "segmentation fault" caused by the elm binary not being compatible with the arm64 architecture. This updates the Dockerfile to use a new command --build-arg PLATFORM=<arm64/amd64> amd64 by default in the build instructions to build a container specifically for your platform.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
INSTALL.md
to include commands specific to Apple Silicon architecture, improving usability for a wider audience.Chores
docker-compose.yml
to utilize dynamic platform specifications, making the configuration more adaptable across environments.