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

dockerTools.*: Assertion against building for Darwin #77952

Merged
merged 1 commit into from Jan 31, 2020

Conversation

infinisil
Copy link
Member

@infinisil infinisil commented Jan 17, 2020

Motivation for this change

Building a docker image with Darwin binaries just yields a confusing
error when ran:

standard_init_linux.go:211: exec user process caused "exec format error"

This change prevents people from building such images in the first place

This PR is sponsored by Niteo ✨

Ping @grahamc @zupo

Things done
  • Verified that the assertion is not triggered on Linux
  • Verified that the assertion is triggered on macOS

@matthewbauer
Copy link
Member

Does it also need to be x86_64 architecture?

@infinisil
Copy link
Member Author

According to https://stackoverflow.com/a/52845392/6605742 it doesn't, so I now switched to buildPlatform.isLinux

@infinisil
Copy link
Member Author

infinisil commented Jan 17, 2020

Question to @grahamc: Is it possible to build a Linux docker image on macOS by doing it inside a Linux qemu VM? Because then this assertion might prevent people from doing that.

Edit: vmTools.runInLinuxVM might be able to do that

@matthewbauer
Copy link
Member

If you change this from buildPlatform to hostPlatform, it should allow you to still cross compile a kernel from macOS to Linux and put it in a Docker image.

@infinisil
Copy link
Member Author

Just noticing the different platforms here too, including darwin: https://github.com/moby/moby/blob/master/image/spec/v1.2.md#image-json-field-descriptions

@tomberek
Copy link
Contributor

It should remain possible to build the container on OSX, but get all binaries from cache or a nix-docker builder.

@infinisil
Copy link
Member Author

Ah so the above link is about moby, which is like a container abstraction thing, the os parameter doesn't matter for Docker since Docker can be installed on all operating systems. The architecture parameter does matter for Docker, as I believe it needs to be the same between the linux binaries within docker and the architecture docker runs on. But anyways, this doesn't seem relevant for this PR, docker builds linux containers, and that's what this PR restricts it to.

I initially confused hostPlatform and buildPlatform, I switched it around so that it's correct.

@infinisil
Copy link
Member Author

@matthewbauer Looking good now?

@grahamc
Copy link
Member

grahamc commented Jan 20, 2020

Note that Windows Docker images can be built for Windows, executing Windows binaries, too.

@grahamc
Copy link
Member

grahamc commented Jan 20, 2020

(NO idea if anybody is using dockerTools to do that, so I wouldn't hold this PR back on that basis ... but maybe this PR would get reverted on that basis :) )

Building a docker image with darwin binaries just yields a confusing
error when ran:

  standard_init_linux.go:211: exec user process caused "exec format error"

This change prevents people from building such images in the first place
@infinisil
Copy link
Member Author

Hm yeah, I now changed it so:

  • It only prevents build for darwin, instead of only allowing for linux
  • It uses meta.badPlatforms instead of an assertion

@infinisil
Copy link
Member Author

I'll go ahead and merge this soon if nobody has any complaints

@infinisil infinisil changed the title dockerTools.*: Assertion against building for non-Linux dockerTools.*: Assertion against building for Darwin Jan 31, 2020
@infinisil infinisil merged commit 0a351c3 into NixOS:master Jan 31, 2020
@infinisil infinisil deleted the docker-linux-exclusive branch January 31, 2020 20:17
anna328p pushed a commit to anna328p/nixpkgs that referenced this pull request Feb 2, 2020
dockerTools.*: Assertion against building for Darwin
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants