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

Error due to path separator inconsistency on windows #1329

Closed
cchafer opened this issue Apr 14, 2020 · 11 comments · Fixed by #1343
Closed

Error due to path separator inconsistency on windows #1329

cchafer opened this issue Apr 14, 2020 · 11 comments · Fixed by #1343

Comments

@cchafer
Copy link
Contributor

cchafer commented Apr 14, 2020

Expected behaviour

sbt stage executed on windows should generate a consistent dockerfile, in terms of paths description.

Actual behaviour

Some paths in the output Dockerfile mix / and \, resulting in an error when building the image.

Information

  • sbt-native-packager: 1.7.0
  • sbt version: 1.3.8
  • build system: Windows
  • Package built: docker

Reproduction

I'm using a custom start script, called "startDocker.sh" and put in /src/universal/bin

When adding a custom entrypoint in sbt

dockerEntrypoint := Seq("/opt/docker/bin/startDocker.sh")

the result on macos / linux contains (as expected):

RUN ["chmod", "u+x,g+x", "/1/opt/docker/bin/startDocker.sh"]

but on windows, the same line is

RUN ["chmod", "u+x,g+x", "/1/opt/docker/bin\startDocker.sh"]

I would not bother too much about this if the consequence wasn't docker to return the following error when running docker build (or docker:publish)

Step 12/25 : RUN ["chmod", "u+x,g+x", "/opt/docker/bin\startDocker.sh"]
 ---> Running in f6b9fec09930
/bin/sh: 1: [chmod,: not found
The command '/bin/sh -c ["chmod", "u+x,g+x", "/opt/docker/bin\startDocker.sh"]' returned a non-zero code: 127
@muuki88
Copy link
Contributor

muuki88 commented Apr 15, 2020

Thanks @cchafer for the detailed bug report ❤️ This doesn't look correct at all. Do you have time and passion to fix this? If so, I could search up some spots that might cause this.

@ppiotrow could this be something that was introduced with the new layering?

@ppiotrow
Copy link
Contributor

Quickly checked the code. Doesn't seem like it could be introduced. I don't have windows to check.
@cchafer could you try to reproduce on sbt-native-packager:1.6.1 so we know when bug was introduced?

@nigredo-tori
Copy link
Collaborator

AFAICT, this is a combination of two problematic behaviors.

  1. Destination paths for mappings use system path separators. This is the main issue.
  2. ExecCmd assembles the JSON array by hand. This is why sh searches for [chmod instead of chmod.

I don't think either of these is new. Most likely we didn't encounter this issue because no one had bothered generated Dockerfiles on Windows before.

One way we can fix the first issue is by post-processing the mappings in DockerPlugin, and replacing platform-dependent path separators with forward slash. Of course, that would mean we assume *nix environment for the image - and, apparently, Windows images are a thing. Do we need a new setting for this?..

@muuki88
Copy link
Contributor

muuki88 commented Apr 15, 2020

Thanks for taking a quick @ppiotrow 👍

I agree @nigredo-tori , the mappings are the core issue for this.

One way we can fix the first issue is by post-processing the mappings in DockerPlugin, and replacing platform-dependent path separators with forward slash

That would probably the sanest way to do.

Of course, that would mean we assume *nix environment for the image - and, apparently, Windows images are a thing. Do we need a new setting for this?

You mean we store the docker build environment in a setting, e.g.

sealed trait DockerBuildHost
case object Windows
case object Unix
val settingKey dockerBuildHost = setting[DockerBuildHost]("...")

dockerBuildHost = detectOS()

@nigredo-tori
Copy link
Collaborator

nigredo-tori commented Apr 15, 2020

You mean we store the docker build environment in a setting

Pretty much. The name should reference the image rather than the build host, but right now that's distinction without difference. 😄 However, I think we can start with something lower-level:

val dockerImagePathSeparator = settingKey[Char](...)
// we almost always build *nix images.
dockerImagePathSeparator := '/'

Or just don't add this knob until someone actually tries to build a Windows image. I think DockerPlugin relies a lot on various *nix conventions, and fixing that is out of scope of this issue.

@muuki88
Copy link
Contributor

muuki88 commented Apr 15, 2020

I like the lower-level approach. It's much clearer what the purposes is and easier to configure.

The name should reference the image rather than the build host, but right now that's distinction without difference

I understood the issue that on the host system windows things get strange. Not that a docker image is based on windows, right? Docker confuses me so fast 😜

My point is that a lower level key setting should ideally still be configured automatically by detecting the underlying OS to... oh no... You only need this if you build an actual windows docker image.

We take that solution!

@cchafer any chance you want to dig into the depths of the docker plugin 😂

@nigredo-tori
Copy link
Collaborator

I understood the issue that on the host system windows things get strange. Not that a docker image is based on windows, right? Docker confuses me so fast stuck_out_tongue_winking_eye

We actually have 3 environments here:

  1. Whatever is running SBT (I'll call it the SBT host). In this case this is Windows.
  2. Whatever will build the Dockerfile (Docker host).
  3. Whatever the docker image has (image). In this case it's probably some flavor of Linux

The issue here is that on the SBT host the generated mappings use Windows path separators (backslashes). However, the image (where the chown command is executed) expects the Dockerfile to contain *nix path separators (forward slashes). So we need to convert SBT host OS path separators to image OS path separators. We want to be able to configure SBT so that the Dockerfile commands use the path separators in the Dockerfile to what image expects. We have no good way of knowing what those are for the image we're building, but defaulting to *nix separators seems reasonable. Note that the docker host OS is irrelevant here, though it has some corellation with the image OS.

@cchafer
Copy link
Contributor Author

cchafer commented Apr 23, 2020

Sorry for my late reply, Covid doesn't help.
Just my 2 cents : at this time, I think you can't build windows docker images with sbt-native-packager.
The generated dockerfile contains too many *nix specific commands (chmod ?).
I like the idea to be able to add configurations per image, but you may then encounter some requests like "I can't build my <....> windows image, I get an error 'chmod.exe is not an executable'"

This said, if I can help, let me know !

@JessePelton
Copy link

JessePelton commented Apr 24, 2020

I've been wrestling for hours with a problem that may be related, though it has nothing whatever to do with Docker. I want to control the collection of files archived by universal:packageZipTarball on a Windows system. Specifically, I want to archive everything in mappings except the contents of a subdirectory tree. On Windows, the following code doesn't quite work to generate the list of files to be archived.

lazy val zipping = {
  mappings --= (file("excluded/directory") ** "*")
    .get
    .map(file => baseDirectory.value / file.getPath -> file.getPath)
}

Running show mappings in the sbt console makes it clear why: the path for each mapping in the subdirectory has a forward slash for its first directory separator, and any subsequent separators are backslashes: excluded/directory\and\subdirectories.

Consequently I have to employ the following string replacement hack to get the desired behavior.

lazy val zipping = {
  mappings --= (file("excluded/directory") ** "*")
    .get
    .map(file => baseDirectory.value / file.getPath -> file.getPath.replaceFirst("\\\\", "/"))
}

So, while it's true that

Destination paths for mappings use system path separators

that's not the whole problem for me, because system path separators aren't used exclusively. If they were, my hack wouldn't be required.

It might be better in the general case if the destination paths used / consistently. I could live with that; it's easier to understand why replace('\\', '/') would be necessary than replaceFirst("\\\\", "/").

@muuki88
Copy link
Contributor

muuki88 commented Apr 27, 2020

@JessePelton what you describe looks like a bug in sbt itself 😮 as all the code you wrote is not native-packager related.

@nigredo-tori explained the issue with SBT host and docker image path separators very well. So not sure if a fix in sbt would solve this issue too.

@cchafer
Copy link
Contributor Author

cchafer commented May 22, 2020

Hi, just made a PR that (at least) fixes my problem.
I did not add any dockerImagePathSeparator entry since it appears clearly that codebase is not made at all to work on any other system than linux.
Just an exemple to make my point: according to code, making a path is done with
"%s/%s" format (basePath, file).

So what I do is just checking if current system separator is consistent, and if not, replace in result string.

Checked by constructing a DockerFile on Windows, that can then be built on Linux.

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

Successfully merging a pull request may close this issue.

5 participants