-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Fix compilation for GOLANG=(mipsle|mips64le) #4032
Conversation
I think the approach should be different, see this: #4037 |
`libcontainer/dmz_linux.go` is not compiled in for MIPS, as libcontainer/dmz/nolibc/arch.h isn't compatible. Signed-off-by: Akihiro Suda <[email protected]>
9097f98
to
ff239ef
Compare
That proposal doesn't seem compatible with non-makefile tools such as |
//go:build (386 || amd64 || arm || arm64 || loong64 || ppc64le || riscv64 || s390x) && !runc_nodmz | ||
// +build 386 amd64 arm arm64 loong64 ppc64le riscv64 s390x |
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.
As a side note, I'm not sure why we still have old-style +build tags. Those are not needed since Go 1.18 or so.
Will open a PR to remove those.
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.
LGTM
@@ -1,4 +1,5 @@ | |||
//go:build !runc_nodmz |
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.
I don't think we need to add this here -- if we build runc-dmz then we can use it. I will take this along with @rata's suggestion and make a PR to add MIPS binaries to our release artifacts (presumably Debian has the necessary cross-compilers).
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.
add MIPS binaries to our release artifacts
I don't think we have to do this, MIPS is no longer official on our side.
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.
Can we merge this, and discuss your concern in a separate PR?
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.
I think the libct/dmz changes are not needed, it should just compile fine on those arches (like this PR is doing: https://github.com/opencontainers/runc/pull/4076/files).
Other than that, I'm not opposed to this at all. Not sure why we say mips is not supported in the docs (https://github.com/opencontainers/runc/blob/main/docs/spec-conformance.md#architectures) and then do this in the code. But I'm fine with this cross-compile for MIPS.
@@ -1,4 +1,5 @@ | |||
//go:build !runc_nodmz | |||
//go:build (386 || amd64 || arm || arm64 || loong64 || ppc64le || riscv64 || s390x) && !runc_nodmz | |||
// +build 386 amd64 arm arm64 loong64 ppc64le riscv64 s390x |
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.
I don't think we need to add the arches here. IMHO, we should leave this as it was and just make all arches compile, as this PR is doing: https://github.com/opencontainers/runc/pull/4076/files
//go:build !linux || runc_nodmz | ||
// +build !linux runc_nodmz | ||
//go:build !linux || (!386 && !amd64 && !arm && !arm64 && !loong64 && !ppc64le && !riscv64 && !s390x) || runc_nodmz | ||
// +build !linux !386,!amd64,!arm,!arm64,!loong64,!ppc64le,!riscv64,!s390x runc_nodmz |
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.
idem, I think we don't need to add specific arches here, just leave the build tag.
libcontainer/dmz_linux.go
is not compiled in for MIPS, as libcontainer/dmz/nolibc/arch.h isn't compatible.Fixes #4037
Note: MIPS is no longer officially supported by runc maintainers, but still supported by Debian: