Skip to content

feat: support make to build multi-arch - #1647

Merged
zdtsw merged 3 commits into
opendatahub-io:mainfrom
zdtsw-forking:chore_184
Feb 11, 2025
Merged

zdtsw merged 3 commits into
opendatahub-io:mainfrom
zdtsw-forking:chore_184

Conversation

@zdtsw

@zdtsw zdtsw commented Feb 11, 2025

Copy link
Copy Markdown
Member
  • to really use this feature, the underlying host need to support it, one way is qeum
  • for the ones who are on M1/2, need to have PLATFORM as linux/arm64 set locally, since it is not gonna auto detected by go anymore but passing thro TARGET as arg for image build

Description

How Has This Been Tested?

Screenshot or short clip

Merge criteria

  • You have read the contributors guide.
  • Commit messages are meaningful - have a clear and concise summary and detailed explanation of what was changed and why.
  • Pull Request contains a description of the solution, a link to the JIRA issue, and to any dependent or related Pull Request.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

- to really use this feature, the underlying host need to support it, one way is qeum

Signed-off-by: Wen Zhou <wenzhou@redhat.com>
@zdtsw
zdtsw requested review from csams, lburgazzoli and ykaliuta and removed request for StevenTobin February 11, 2025 10:59
Comment thread README.md Outdated
Comment thread README.md
- To build multi-arch image, by config env variablel PLATFORM
```commandline
export PLATFORM=linux/amd64,linux/arm64,linux/ppc64le,linux/s390x
make image

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it really a requirement to get it from environment? I would rather keep it make-only (make PLATFORM=...) than depend of non-specific, but generic name in the environment.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

i set env in a local.mk file, so i do not need to pass PLATFORM everytime i type make.
and if i do not need multiple arch, i just comment out that variable in local.mk
i think we were using local.mk as mentioned in the https://github.com/opendatahub-io/opendatahub-operator/blob/main/docs/troubleshooting.md#using-a-localmk-file-to-override-makefile-variables-for-your-development-environment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i set env in a local.mk file, so i do not need to pass PLATFORM everytime i type make. and if i do not need multiple arch, i just comment out that variable in local.mk i think we were using local.mk as mentioned in the https://github.com/opendatahub-io/opendatahub-operator/blob/main/docs/troubleshooting.md#using-a-localmk-file-to-override-makefile-variables-for-your-development-environment

But local.mk has make variables, not environment in sense of export command. See b41b6aa

@ykaliuta ykaliuta Feb 11, 2025 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Using PLATFORM ?= makes it possible to set the variable from the environment (in unix sense) so can be useful for people who use something like https://direnv.net. But in general it's a common place, so non-namespaced variables there ring the bell.

I'm just wondering what is the intention. Environment works for me as well.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

should i change this to :

  • To build multi-arch image, set variable PLATFORM in your local.mk file, e.g:
PLATFORM=linux/amd64,linux/arm64,linux/ppc64le,linux/s390x

then run

  make image

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I already approved it so let's leave as it is. It's correct with the patch.

Comment thread Makefile
Signed-off-by: Wen Zhou <wenzhou@redhat.com>
@openshift-ci openshift-ci Bot added the lgtm label Feb 11, 2025
@openshift-ci

openshift-ci Bot commented Feb 11, 2025

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ykaliuta

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@biswassri

Copy link
Copy Markdown
Contributor

@zdtsw @ykaliuta I might be wrong but what we resolving here also covers what is in this PR #921 ?

@openshift-ci openshift-ci Bot removed the lgtm label Feb 11, 2025
@openshift-ci

openshift-ci Bot commented Feb 11, 2025

Copy link
Copy Markdown

New changes are detected. LGTM label has been removed.

@ykaliuta

Copy link
Copy Markdown
Contributor

@zdtsw @ykaliuta I might be wrong but what we resolving here also covers what is in this PR #921 ?

Not exactly. That one is more about #1549, to not force binary build to amd64 on whatever build/target architectures.

Here is to set target architecture(s) on whatever build one.

@zdtsw

zdtsw commented Feb 11, 2025

Copy link
Copy Markdown
Member Author

@zdtsw @ykaliuta I might be wrong but what we resolving here also covers what is in this PR #921 ?

921 is not needed as i see and it is only target for go build but not image.

@codecov

codecov Bot commented Feb 11, 2025 •

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 20.35%. Comparing base (5983dd2) to head (547cfd4).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1647   +/-   ##
=======================================
  Coverage   20.35%   20.35%           
=======================================
  Files         163      163           
  Lines       11156    11156           
=======================================
  Hits         2271     2271           
  Misses       8645     8645           
  Partials      240      240           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zdtsw
zdtsw merged commit d2a4faf into opendatahub-io:main Feb 11, 2025
@zdtsw

zdtsw commented Apr 15, 2025

Copy link
Copy Markdown
Member Author

/cherrypick rhoai

@openshift-cherrypick-robot

Copy link
Copy Markdown

@zdtsw: new pull request created: #1867

Details

In response to this:

/cherrypick rhoai

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants