copyExecutable function overwrites file perfmission for non owners

[xerial]

In the current implementation, copyExecutable does not favor the original file permission except the file owner:

  /** Transfers the executable property of `sourceFile` to `targetFile`. */
  def copyExecutable(sourceFile: File, targetFile: File) = {
    val executable = sourceFile.canExecute
    if (executable) targetFile.setExecutable(true) /// equivalent to  setExecutable(executable=true, ownerOnly=true)
  }

For example, if the original file permission is 775 (rwxrwxr-x), and copied by sbt.io.IO.copyExecutable, it will results in 764 (rwxrw-r--) executable flag are removed from the group and others).

We've found this issue in https://github.com/xerial/sbt-pack/issues/135

[xerial]

It seems in sbt 0.13.x, the file permission was retained.

[xerial]

We need to retain the original file permission before setting these flags. I'll create a PR to fix this.

[xerial]

@eed3si9n One question. Should we add preserverWritable flag in addition to preserveExecutable?

[eed3si9n]

If you need more detailed permission, maybe use POSIX permission operations that were added in https://github.com/sbt/io/pull/76?

[xerial]

@eed3si9n Yes. That is an option as well.

If disregarding the source file permission is the expected behavior, sbt-pack sets the permission manually. If not, sbt-io needs to be modified to retain the original permission.

[xerial]

For preserving the original file permission, setting preserveExecutable=false conflicts with preservePermssion (like cp -p) behavior. That's my concern.

[eed3si9n]

In sbt 0.13 there was no parameter on copyFile to set the execution flag.

If disregarding the source file permission is the expected behavior

Disregarding the source file permission, sort of is the behavior of File#setExecutable flag. Looking at the history, this was introduced as https://github.com/sbt/io/pull/53 in sbt 1 to as part of forward porting (https://github.com/sbt/sbt/issues/2384, https://github.com/sbt/sbt/commit/1e61b9acf34b7d89a54a5adc021cc37cd3ecd0ef).

In reality, as you point out, the result came out to be inconsistent and clumsy at least on the system that you're testing (macOS?). I am not sure how we should proceed on this. I think not copying when perserveExecutable = false would even be more confusing if the existing code relies on the behavior.

At the least, copyExecutable should NOT lose permission.

/cc @dwijnand

[eed3si9n]

It seems in sbt 0.13.x, the file permission was retained.

Is this your observation from running the code? I haven't tested it, but I am not seeing the code that's responsible for transferring the permissions in 0.13 - https://github.com/sbt/sbt/blob/v0.13.17-RC2/util/io/src/main/scala/sbt/IO.scala#L647-L669

[xerial]

@eed3si9n 0.13 behavior is reported by a sbt-pack user, which simply uses IO.copyFile.

I submitted a PR for retaining the original file permission #126 .

[eed3si9n]

I just tested on macOS, and I was not able to reproduce the behavior that preserves the permission in 0.13.

scala> IO.copyFile(new File("/tmp/io/A.sh"), new File("/tmp/io/B.sh"), false)

scala> IO.copyFile(new File("/tmp/io/A.sh"), new File("/tmp/io/C.sh"), true)

Here's the result:

-rwxrwxrwx   1 eed3si9n  wheel    0 Jan 23 18:08 A.sh*
-rw-r--r--   1 eed3si9n  wheel    0 Jan 23 18:10 B.sh
-rw-r--r--   1 eed3si9n  wheel    0 Jan 23 18:08 C.sh

[xerial]

@eed3si9n OK. So this behavior has been the same for a long time. It means #126 is good to have, but it may break the behavior of existing sbt plugins.

[eed3si9n]

Right. also it makes copying POSIX specific, so Windows ppl might run into issues.

[eed3si9n]

Pending question here - https://github.com/xerial/sbt-pack/issues/135#issuecomment-359965744

[dskrvk]

@eed3si9n I responded in the referenced issue.