Skip to content

Allow building on unsupported architectures#15

Closed
yselkowitz wants to merge 1 commit intoopenshift:openshift-4.2from
multi-arch:openshift-4.2
Closed

Allow building on unsupported architectures#15
yselkowitz wants to merge 1 commit intoopenshift:openshift-4.2from
multi-arch:openshift-4.2

Conversation

@yselkowitz
Copy link
Copy Markdown

@openshift-ci-robot
Copy link
Copy Markdown

Hi @yselkowitz. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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/test-infra repository.

@openshift-ci-robot openshift-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 11, 2019
@openshift-ci-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: yselkowitz
To complete the pull request process, please assign hexfusion
You can assign the PR to them by writing /assign @hexfusion in a comment when ready.

The full list of commands accepted by this bot can be found 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

@hexfusion
Copy link
Copy Markdown

@yselkowitz what arch are you trying to support? Not sure how I feel about carrying a patch for this, all etcd code is upstream minus CI tooling.

@yselkowitz
Copy link
Copy Markdown
Author

@hexfusion, sorry, I didn't see a Dockerfile.rhel upstream. In which branch is it?

@hexfusion
Copy link
Copy Markdown

hexfusion commented Jul 11, 2019

@hexfusion, sorry, I didn't see a Dockerfile.rhel upstream. In which branch is it?

Well, it was my misunderstanding actually. Have you tested this? I get

2019-07-11 12:24:00.054992 W | pkg/flags: unrecognized environment variable ETCD_UNSUPPORTED_ARCH=`uname -m`

Also this would set this ENV for every build this and might confuse folks. Why can't you pass the ENV during runtime?

@hexfusion
Copy link
Copy Markdown

/ok-to-test

just so we can review output

@openshift-ci-robot openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 11, 2019
@hexfusion
Copy link
Copy Markdown

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 11, 2019
There is no apparent way to set GOARCH appropriately in the runtime
container (since go is not present), therefore it is necessary to relax
the required value of ETCD_UNSUPPORTED_ARCH.
@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 16, 2019
@hexfusion
Copy link
Copy Markdown

status update, etcd team is looking into this.

@yselkowitz
Copy link
Copy Markdown
Author

Could we get an update on this please? This is a prerequisite of OpenShift on IBM Z.

@hexfusion
Copy link
Copy Markdown

hexfusion commented Aug 28, 2019 via email

@hexfusion
Copy link
Copy Markdown

Closed in favor of openshift/machine-config-operator#1092

@hexfusion hexfusion closed this Aug 29, 2019
@yselkowitz
Copy link
Copy Markdown
Author

@hexfusion unfortunately that is insufficient. Not only is this a runtime issue, but actually affects the build of the container image as well.

The container does RUN make build, which not only compiles etcd but also runs it as a test. Therefore, as it currently stands, the image build itself still fails.

In order to fix the image build, either we can add ENV ETCD_UNSUPPORTED_ARCH=$(go env GOARCH) where the first part of my patch would have set =1, or the Makefile needs to be modified to set that in the make environment, or the upstream code needs to be relaxed as originally proposed.

Please reopen so that a new patch can be proposed accordingly.

@hexfusion
Copy link
Copy Markdown

hexfusion commented Aug 29, 2019

In order to fix the image build, either we can add ENV ETCD_UNSUPPORTED_ARCH=$(go env GOARCH) where the first part of my patch would have set =1, or the Makefile needs to be modified to set that in the make environment, or the upstream code needs to be relaxed as originally proposed.

That makes sense but we do not want to carry a patch to do this. The build process itself has no care about arch but the make tooling showed a limitation. I have adjusted the build process[1] as an alternative allowing build on any arch. The MCO[2] change will allow your binaries to run.

[1] #17
[2] openshift/machine-config-operator#1092

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

Labels

do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants