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

Feature/ocu 184 allow binaries to be built for architectures different than linux/amd64 #339

Conversation

andream16
Copy link
Contributor

@andream16 andream16 commented Sep 5, 2024

As per title, allow to customise the built images and binaries for 🍎 people and documenting the utilisation.

We basically use --platform when building to specify for which platform we want to build an image. At the same time we can customise for which architecture the Go binaries should be built for.


Based on the supplied ARCH and OS, a binary is created in a self-documented sub-folder, as follows:

Screenshot 2024-09-09 at 15 04 16


It would be nice to collapse the different Dockerfiles and Makefiles in components into a single one, so such customisation can be centralised. I'll start a thread in the chat about it.


Tested locally:

  • make components -> builds with linux/amd64 by default. Running the containers works fine.
  • make GOOS=linux GOARCH=arm64 CONTAINER_OS_ARCH=linux/arm64 components -> builds with linux/arm64. Running the containers work.
  • make GOOS=darwin GOARCH=arm64 component-binaries -> build with darwin/arm64 and I can exec the binaries fine on my machine.

I would appreciate if you can try these commands yourself and if you have other pointers to what to test.


Minor: bumping remark-cli to v12.0.1 to suppress formatting errors.

@ptzianos
Copy link
Contributor

ptzianos commented Sep 5, 2024

@andream16 yeah I think it would be important to also build the binaries with the correct platform otherwise the containers will fail if we try to run them in a foreign architecture

echo "${1}" | grep -Eq ^components/producers/.*$ && executable="${executable}-parser" || true
echo "${1}" | grep -Eq ^components/enrichers/.*$ && executable="${executable}" || true
echo "${1}" | grep -Eq ^components/consumers/.*$ && executable="${executable}" || true
echo $dir_name | grep -Eq ^components/producers/.*$ && executable="${executable}-parser" || true
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: for style consistency can you convert your variables to be in the form ${dir_name} instead of $dirname. I'm hoping that we will soon have shellcheck linting the repo too: https://www.shellcheck.net/wiki/SC2250

Makefile Show resolved Hide resolved
CONTAINER_OS_ARCH=linux/amd64
# Allow to independently customise go OS and ARCH flags independently and following the standard practices.
# Defaulting to linux/amd64 as per CONTAINER_OS_ARCH.
GOOS=linux
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks like it could cause silent issues for ppl building the binaries to just run them locally. I think it would be better to just set them to $(shell go env GOOS) and if someone wants to override them via a environment variable, they can

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had a discussion offline and we decided to use these defaults as they work well in both linux/mac environments.

Same goes for containers platform. We can't assume the OS for docker as darwin (from mac) is not supported.

@ptzianos
Copy link
Contributor

@andream16 just a couple of nits as comments, would you also mind cleaning up the history of the PR a little bit. the last commit contains both README fixes and package.json updates and a couple of the initial commits touch the same things again and again because we had a bunch of discussions about this as it was in flight and they can be squashed

@andream16 andream16 closed this Sep 11, 2024
@andream16
Copy link
Contributor Author

Declined in favour of #343 with better git commits

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.

3 participants