Don't set cross-compilation variables in Makefile #4346
Conversation
kolyshkin
force-pushed
the
xc
branch
6 times, most recently
from
bad6cd7
to
12a0905
Compare
This feature (cc_platform.mk and its sourcing from Makefiles) was added by commit dac4171 ("runc-dmz: reduce memfd binary cloning cost with small C binary") as part of dmz work. From what I see, this is not needed for `make releaseall` as we already set these variables using `set_cross_vars` shell function. My best guess is this was mostly needed for testing (and, apparently, for cross-i386 CI job). The big issue though is these variables are specific to our cross-compile environment, and setting it from Makefile breaks other scenarios: - native builds on non-x86_64 platforms; - using other cross-compilation tools. One such example is Moby build which uses https://github.com/tonistiigi/xx. Let's remove this from the Makefiles, and slightly modify the script which sets those variables so it can be used from the command line. Signed-off-by: Kir Kolyshkin <[email protected]>
[@kolyshkin: hijacked this to test opencontainers/runc#4346] rc.2 release notes: https://github.com/opencontainers/runc/releases/tag/v1.2.0-rc.2 rc.1 release notes: https://github.com/opencontainers/runc/releases/tag/v1.2.0-rc.1 Breaking changes and deprecations are included below; Breaking changes: Several aspects of how mount options work has been adjusted in a way that could theoretically break users that have very strange mount option strings. This was necessary to fix glaring issues in how mount options were being treated. The key changes are: - Mount options on bind-mounts that clear a mount flag are now always applied. Previously, if a user requested a bind-mount with only clearing options (such as rw,exec,dev) the options would be ignored and the original bind-mount options would be set. Unfortunately this also means that container configurations which specified only clearing mount options will now actually get what they asked for, which could break existing containers (though it seems unlikely that a user who requested a specific mount option would consider it "broken" to get the mount options they asked foruser who requested a specific mount option would consider it "broken" to get the mount options they asked for). This also allows us to silently add locked mount flags the user did not explicitly request to be cleared in rootless mode, allowing for easier use of bind-mounts for rootless containers. - Container configurations using bind-mounts with superblock mount flags (i.e. filesystem-specific mount flags, referred to as "data" in mount(2), as opposed to VFS generic mount flags like MS_NODEV) will now return an error. This is because superblock mount flags will also affect the host mount (as the superblock is shared when bind-mounting), which is obviously not acceptable. Previously, these flags were silently ignored so this change simply tells users that runc cannot fulfil their request rather than just ignoring it. Deprecated - runc option --criu is now ignored (with a warning), and the option will be removed entirely in a future release. Users who need a non-standard criu binary should rely on the standard way of looking up binaries in $PATH. - runc kill option -a is now deprecated. Previously, it had to be specified to kill a container (with SIGKILL) which does not have its own private PID namespace (so that runc would send SIGKILL to all processes). Now, this is done automatically. - github.com/opencontainers/runc/libcontainer/user is now deprecated, please use github.com/moby/sys/user instead. It will be removed in a future release. Signed-off-by: Sebastiaan van Stijn <[email protected]> Signed-off-by: Kir Kolyshkin <[email protected]>
|
ptal @cyphar |
|
Grrr, GitHub ate my comment. I added To be clear, I agree that the duplication is annoying. Maybe we could remove For the |
|
Hmm, I had a wrong assumption that What's wrong is it assumes native compilation here: Line 14 in 3778ae6 It doesn't work when xx-go --wrap sets up To work around this, we can check the origin of GOENV like @tonistiigi suggested in moby/moby#48160 (comment). Opened #4349. |
It's even easier -- use xx to set CC and |
This is an alternative to #4345, fixing the issue with using new runc for Docker (moby/moby#47666)
This feature (cc_platform.mk and its sourcing from Makefiles) was added by commit dac4171 ("runc-dmz: reduce memfd binary cloning cost with small C binary") as part of dmz work.
From what I see, this is not needed for
make releaseallas we already set these variables usingset_cross_varsshell function. My best guess is this was mostly needed for testing.The big issue though is these variables are guessed based on our cross-compilation environment, meaning they won't work in other cases (such as native compilation on non-x86_64 platforms, or using other cross-compilation tools). One such example is Moby build which uses https://github.com/tonistiigi/xx.
Besides, it's a duplication of effort in scripts/lib.sh, which is prone to error.
Let's remove this from the Makefiles, and rework the script which sets those variables so it can be used for other scenarios as well.
For example, to test cross-compilation locally, use