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

Fix empty folder missing problem when zip files. #330

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open
15 changes: 11 additions & 4 deletions build.mill
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ val communityBuildDottyVersion = sys.props.get("dottyVersion").toList
val scala213Version = "2.13.14"

val scalaVersions = Seq(
"3.3.1",
"2.12.17",
"3.3.3",
"2.12.19",
counter2015 marked this conversation as resolved.
Show resolved Hide resolved
scala213Version
) ++ communityBuildDottyVersion

Expand All @@ -26,6 +26,7 @@ object Deps {
val sourcecode = ivy"com.lihaoyi::sourcecode::0.4.2"
val utest = ivy"com.lihaoyi::utest::0.8.4"
val expecty = ivy"com.eed3si9n.expecty::expecty::0.16.0"
val collectionCompact = ivy"org.scala-lang.modules::scala-collection-compat:2.9.0"
def scalaReflect(scalaVersion: String) = ivy"org.scala-lang:scala-reflect:$scalaVersion"
def scalaLibrary(version: String) = ivy"org.scala-lang:scala-library:${version}"
}
Expand All @@ -39,7 +40,12 @@ trait AcyclicModule extends ScalaModule {
}
def compileIvyDeps = acyclicDep
def scalacPluginIvyDeps = acyclicDep
def scalacOptions = super.scalacOptions() ++ acyclicOptions()
def scalacOptions = super.scalacOptions() ++ acyclicOptions() ++ Seq(
"-deprecation",
"-feature",
"-language:implicitConversions",
"-language:higherKinds",
)
}

trait SafeDeps extends ScalaModule {
Expand Down Expand Up @@ -112,7 +118,7 @@ trait OsLibModule
}

trait OsModule extends OsLibModule { outer =>
def ivyDeps = Agg(Deps.geny)
def ivyDeps = Agg(Deps.geny, Deps.collectionCompact)
override def compileIvyDeps = T {
val scalaReflectOpt = Option.when(!ZincWorkerUtil.isDottyOrScala3(scalaVersion()))(
Deps.scalaReflect(scalaVersion())
Expand Down Expand Up @@ -165,6 +171,7 @@ trait OsModule extends OsLibModule { outer =>
}
}


object os extends Module {

object jvm extends Cross[OsJvmModule](scalaVersions)
Expand Down
4 changes: 2 additions & 2 deletions os/src/GlobInterpolator.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ package os

object GlobInterpolator {
class Interped(parts: Seq[String]) {
def unapplySeq(s: String) = {
val Seq(head, tail @ _*) = parts.map(java.util.regex.Pattern.quote)
def unapplySeq(s: String): Option[List[String]] = {
val Seq(head, tail @ _*) = parts.map(java.util.regex.Pattern.quote): @unchecked

val regex = head + tail.map("(.*)" + _).mkString
regex.r.unapplySeq(s)
Expand Down
22 changes: 11 additions & 11 deletions os/src/ListOps.scala
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import java.nio.file.attribute.BasicFileAttributes
object list extends Function1[Path, IndexedSeq[Path]] {
def apply(src: Path, sort: Boolean = true): IndexedSeq[Path] = {
val arr = stream(src).toArray[Path]
if (sort) arr.sorted
if (sort) arr.sorted.toIndexedSeq
else arr
}
def apply(src: Path): IndexedSeq[Path] = apply(src, true).toIndexedSeq
Expand Down Expand Up @@ -53,7 +53,7 @@ object list extends Function1[Path, IndexedSeq[Path]] {
* saving time as compared to filtering them after the fact.
*
* By default, the paths are returned as a pre-order traversal: the enclosing
* folder is occurs first before any of it's contents. You can pass in `preOrder =
* folder occurs first before any of its contents. You can pass in `preOrder =
* false` to turn it into a post-order traversal, such that the enclosing folder
* occurs last after all it's contents.
*
Expand All @@ -76,12 +76,12 @@ object walk {
* you want `preOrder` to be `true` so the folder gets
* created first.
*
* @param followLinks Whether or not to follow symlinks while walking; defaults
* @param followLinks Whether to follow symlinks while walking; defaults
* to false
*
* @param maxDepth The max depth of the tree you wish to walk; defaults to unlimited
*
* @param includeTarget Whether or not to include the given path as part of the walk.
* @param includeTarget Whether to include the given path as part of the walk.
* If `true`, does not raise an error if the given path is a
* simple file and not a folder
*/
Expand Down Expand Up @@ -109,12 +109,12 @@ object walk {
* you want `preOrder` to be `true` so the folder gets
* created first.
*
* @param followLinks Whether or not to follow symlinks while walking; defaults
* @param followLinks Whether to follow symlinks while walking; defaults
* to false
*
* @param maxDepth The max depth of the tree you wish to walk; defaults to unlimited
*
* @param includeTarget Whether or not to include the given path as part of the walk.
* @param includeTarget Whether to include the given path as part of the walk.
* If `true`, does not raise an error if the given path is a
* simple file and not a folder
*/
Expand Down Expand Up @@ -145,12 +145,12 @@ object walk {
* you want `preOrder` to be `true` so the folder gets
* created first.
*
* @param followLinks Whether or not to follow symlinks while walking; defaults
* @param followLinks Whether to follow symlinks while walking; defaults
* to false
*
* @param maxDepth The max depth of the tree you wish to walk; defaults to unlimited
*
* @param includeTarget Whether or not to include the given path as part of the walk.
* @param includeTarget Whether to include the given path as part of the walk.
* If `true`, does not raise an error if the given path is a
* simple file and not a folder
*/
Expand Down Expand Up @@ -179,12 +179,12 @@ object walk {
* you want `preOrder` to be `true` so the folder gets
* created first.
*
* @param followLinks Whether or not to follow symlinks while walking; defaults
* @param followLinks Whether to follow symlinks while walking; defaults
* to false
*
* @param maxDepth The max depth of the tree you wish to walk; defaults to unlimited
*
* @param includeTarget Whether or not to include the given path as part of the walk.
* @param includeTarget Whether to include the given path as part of the walk.
* If `true`, does not raise an error if the given path is a
* simple file and not a folder
*/
Expand Down Expand Up @@ -228,7 +228,7 @@ object walk {
// rather than relying on `walkFileTree`, because `walkFileTree`
// does the unintuitive thing when the `path` being walked is a
// symlink to a directory (it just returns the symlink path and
// does not walking)
// does not walk)
val ds = Files.newDirectoryStream(pathNIO)
val iter = ds.iterator()
try {
Expand Down
4 changes: 2 additions & 2 deletions os/src/Path.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ package os

import java.net.URI
import java.nio.file.Paths
import collection.JavaConverters._
import scala.jdk.CollectionConverters._
import scala.language.implicitConversions
import acyclic.skipped
import os.PathError.{InvalidSegment, NonCanonicalLiteral}
Expand Down Expand Up @@ -212,7 +212,7 @@ object BasePath {
}
}
def chunkify(s: java.nio.file.Path) = {
import collection.JavaConverters._
import scala.jdk.CollectionConverters._
s.iterator().asScala.map(_.toString).filter(_ != ".").filter(_ != "").toArray
}
}
Expand Down
2 changes: 1 addition & 1 deletion os/src/ProcessOps.scala
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package os

import collection.JavaConverters._
import scala.jdk.CollectionConverters._
import java.lang.ProcessBuilder.Redirect
import os.SubProcess.InputStream
import java.io.IOException
Expand Down
2 changes: 1 addition & 1 deletion os/src/ReadWriteOps.scala
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ object write {
) = {
checker.value.onWrite(target)

import collection.JavaConverters._
import scala.jdk.CollectionConverters._
val permArray: Array[FileAttribute[_]] =
if (perms == null) Array.empty
else Array(PosixFilePermissions.asFileAttribute(perms.toSet()))
Expand Down
18 changes: 8 additions & 10 deletions os/src/SubProcess.scala
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ sealed trait ProcessLike extends java.lang.AutoCloseable {

/**
* Wait up to `millis` for the [[ProcessLike]] to terminate and all stdout and stderr
* from the subprocess to be handled. By default waits indefinitely; if a time
* from the subprocess to be handled. By default, waits indefinitely; if a time
* limit is given, explicitly destroys the [[ProcessLike]] if it has not completed by
* the time the timeout has occurred.
*
Expand Down Expand Up @@ -152,7 +152,7 @@ class SubProcess(
* @param async set this to `true` if you do not want to wait on the subprocess exiting
* @param shutdownGracePeriod use this to override the default wait time for the subprocess
* to gracefully exit before destroying it forcibly. Defaults to the `shutdownGracePeriod`
* that was used to spawned the process, but can be set to 0
* that was used to spawn the process, but can be set to 0
* (i.e. force exit immediately) or -1 (i.e. never force exit)
* or anything in between. Typically defaults to 100 milliseconds.
*/
Expand Down Expand Up @@ -183,7 +183,7 @@ class SubProcess(
/**
* Alias for [[destroy]]
*/
def close() = wrapped.destroy()
def close(): Unit = wrapped.destroy()

/**
* Wait up to `millis` for the subprocess to terminate, by default waits
Expand Down Expand Up @@ -296,7 +296,7 @@ object SubProcess {
out.toByteArray
}

override def close() = wrapped.close()
override def close(): Unit = wrapped.close()
}
}

Expand All @@ -314,7 +314,7 @@ class ProcessPipeline(
/**
* String representation of the pipeline.
*/
def commandString = processes.map(_.wrapped.toString).mkString(" | ")
def commandString: String = processes.map(_.wrapped.toString).mkString(" | ")

private[os] val brokenPipeHandler: Option[Thread] = brokenPipeQueue.map { queue =>
new Thread(
Expand All @@ -326,7 +326,7 @@ class ProcessPipeline(
if (brokenPipeIndex == processes.length) { // Special case signaling finished pipeline
pipelineRunning = false
} else {
processes(brokenPipeIndex).destroyForcibly()
processes(brokenPipeIndex).destroy(shutdownGracePeriod = 0)
}
}
new Thread(
Expand Down Expand Up @@ -354,9 +354,7 @@ class ProcessPipeline(
*/
override def exitCode(): Int = {
if (pipefail)
processes.map(_.exitCode())
.filter(_ != 0)
.headOption
processes.map(_.exitCode()).find(_ != 0)
.getOrElse(0)
else
processes.last.exitCode()
Expand All @@ -383,7 +381,7 @@ class ProcessPipeline(
* All processes in the pipeline are force-destroyed.
*/
override def destroyForcibly(): Unit = {
processes.foreach(_.destroyForcibly())
processes.foreach(_.destroy(shutdownGracePeriod = 0))
}

/**
Expand Down
12 changes: 8 additions & 4 deletions os/src/ZipOps.scala
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ import java.net.URI
import java.nio.file.{FileSystem, FileSystems, Files}
import java.nio.file.attribute.{BasicFileAttributeView, FileTime, PosixFilePermissions}
import java.util.zip.{ZipEntry, ZipFile, ZipInputStream, ZipOutputStream}
import scala.collection.JavaConverters._
import scala.jdk.CollectionConverters._

import scala.util.matching.Regex

object zip {
Expand Down Expand Up @@ -102,6 +103,7 @@ object zip {
makeZipEntry0(path, source.dest.getOrElse(os.sub) / path.subRelativeTo(source.src))
}
}
makeZipEntry0(source.src, source.dest.getOrElse(os.sub / source.src.last))
} else if (shouldInclude(source.src.last, excludePatterns, includePatterns)) {
makeZipEntry0(source.src, source.dest.getOrElse(os.sub / source.src.last))
}
Expand Down Expand Up @@ -153,17 +155,19 @@ object zip {
val mtimeOpt = if (preserveMtimes) Some(os.mtime(file)) else None

val fis = if (os.isFile(file)) Some(os.read.inputStream(file)) else None
try makeZipEntry0(sub, fis, mtimeOpt, zipOut)

val path = if (os.isDir(file)) sub.toString + "/" else sub.toString
try makeZipEntry0(path, fis, mtimeOpt, zipOut)
finally fis.foreach(_.close())
}

private def makeZipEntry0(
sub: os.SubPath,
path: String,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we using String here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because treats a ZipEntry as directory if its name ends with "/" characters.
Firstly I want to add end "/" character, but in os.Path it's not valid.

Copy link
Author

@counter2015 counter2015 Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it's not a good way to implement, since it breaks the type check inside os.Path, could you give me some advice ?
should I rewrite another method to handle folders such like makeZipFolderEntry0 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to keep the conversion to string and appending of / inside makeZipEntry0. is: Option should already tell us whether it's a file or directory I think

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. But I'm afraid This might be a little slightly fragile assumption, since it's relied on

val fis = if (os.isFile(file)) Some(os.read.inputStream(file)) else None

is: Option[java.io.InputStream],
preserveMtimes: Option[Long],
zipOut: ZipOutputStream
) = {
val zipEntry = new ZipEntry(sub.toString)
val zipEntry = new ZipEntry(path)

preserveMtimes match {
case Some(mtime) => zipEntry.setTime(mtime)
Expand Down
2 changes: 1 addition & 1 deletion os/test/src-jvm/ZipOpJvmTests.scala
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import utest._
import java.nio.file.attribute.FileTime
import java.nio.file.{Files, Paths}
import java.util.zip.ZipFile
import scala.collection.JavaConverters._
import scala.jdk.CollectionConverters._

object ZipOpJvmTests extends TestSuite {

Expand Down
25 changes: 22 additions & 3 deletions os/test/src/ZipOpTests.scala
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ object ZipOpTests extends TestSuite {
dest = wd / "zipByExcludingCertainFiles"
)
val paths = os.walk(outputZipFilePath).sorted
val expected = Seq(wd / "zipByExcludingCertainFiles/File.amx")
val expected = Seq(wd / "zipByExcludingCertainFiles/File.amx", wd / "zipByExcludingCertainFiles")
assert(paths == expected)
}

Expand Down Expand Up @@ -142,7 +142,7 @@ object ZipOpTests extends TestSuite {
// Unzip file to a destination folder
val listedContents = os.unzip.list(source = wd / zipFileName).toSeq

val expected = Seq(os.sub / "File.txt", os.sub / "one.txt")
val expected = Seq(os.sub / "File.txt", os.sub / "one.txt", os.sub / "folder1")
assert(listedContents == expected)
}

Expand Down Expand Up @@ -170,7 +170,8 @@ object ZipOpTests extends TestSuite {
val paths = os.walk(unzippedFolder)
val expected = Seq(
wd / "unzipAllExceptExcludingCertainFiles/File.txt",
wd / "unzipAllExceptExcludingCertainFiles/one.txt"
wd / "unzipAllExceptExcludingCertainFiles/one.txt",
wd / "folder1",
)

assert(paths == expected)
Expand Down Expand Up @@ -222,5 +223,23 @@ object ZipOpTests extends TestSuite {
assert(file2Content == "Content of file2")
}

test("emptyFolder") - prep { wd =>
val zipFileName = "zipCheckEmptyDirectory.zip"
val zipFile = os.zip(
dest = wd / zipFileName,
sources = Seq(
wd / "emptyFolder",
wd / "File.txt"
)
)

val unzippedFolder = os.unzip(
source = wd / zipFileName,
dest = wd / "unzipped-empty-directory"
)

os.walk(unzippedFolder).foreach(println)
assert(os.exists(unzippedFolder / "emptyFolder"))
}
}
}
2 changes: 1 addition & 1 deletion os/watch/src/WatchServiceWatcher.scala
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import java.nio.file.StandardWatchEventKinds.{ENTRY_CREATE, ENTRY_DELETE, ENTRY_
import com.sun.nio.file.{ExtendedWatchEventModifier, SensitivityWatchEventModifier}

import scala.collection.mutable
import collection.JavaConverters._
import scala.jdk.CollectionConverters._
import scala.util.Properties.isWin

class WatchServiceWatcher(
Expand Down
Loading