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

Dockerfile: fix typo in build vs. target platform #18

Merged
merged 1 commit into from Aug 4, 2023

Conversation

jpmcb
Copy link
Member

@jpmcb jpmcb commented Aug 4, 2023

Description

This fixes a problem with cross building the docker image where the wrong rust toolchain would be used resulting in:

exec /bin/sh: exec format error

and also getting the wrong architecture dependencies (like onnxruntime and lib-ssl deps)


Essentially, this uses the target architecture as the base image and the docker buildx's cross architecture buildkit to get all the right bits for the target architecture.

This significantly slows down builds (gotta love rust?) since this now builds everything across architectures and gets cross architecture dependencies. I'm not sure if this is the best approach but I don't see a great alternative since there are cross system dependencies that need to also match the right architecture, i.e., the onnx runtime.

Simply doing

rustup target add --toolchain <toolchain> <target>...

will only get us the correct rust binary, not the right other dependencies.

Anyways, if someone has any ideas, would love to hear them!!

What type of PR is this? (check all applicable)

  • πŸ• Feature
  • πŸ› Bug Fix
  • πŸ“ Documentation Update
  • 🎨 Style
  • πŸ§‘β€πŸ’» Code Refactor
  • πŸ”₯ Performance Improvements
  • βœ… Test
  • πŸ€– Build
  • πŸ” CI
  • πŸ“¦ Chore (Release)
  • ⏩ Revert

Related Tickets & Documents

Related to #15

Mobile & Desktop Screenshots/Recordings

N/a

Added tests?

  • πŸ‘ yes
  • πŸ™… no, because they aren't needed
  • πŸ™‹ no, because I need help

Added to documentation?

  • πŸ“œ README.md
  • πŸ““ docs.opensauced.pizza
  • πŸ• dev.to/opensauced
  • πŸ“• storybook
  • πŸ™… no documentation needed

[optional] Are there any post-deployment tasks we need to perform?

Already built and tested in the cloud with a cross architecture machine.

[optional] What gif best describes this PR or how it makes you feel?

Signed-off-by: John McBride <john@opensauced.pizza>
@Anush008
Copy link
Member

Anush008 commented Aug 4, 2023

Hey. From a GitHub issue on Docker, I found that TARGETPLATFORM is the default value for --platform.

So maybe we should be able to just do
FROM rust:1.71-slim-bullseye AS builder.

@jpmcb
Copy link
Member Author

jpmcb commented Aug 4, 2023

πŸ‘€ ah good call, could work too. I guess it just depend son how verbose we want to be with it. I usually lean toward being pretty explicit just to reduce confusion. What do you think?

@Anush008
Copy link
Member

Anush008 commented Aug 4, 2023

Now when I think about it, figuring out the default value for --platform isn't straightforward or intuitive anyways. Explicitly specifying it will make it clearer. πŸ‘

@jpmcb jpmcb merged commit afc4d19 into open-sauced:alpha Aug 4, 2023
1 check passed
@jpmcb jpmcb deleted the dockerfile-fix branch August 4, 2023 14:43
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