Patch zip library to not use temp files (digital-asset#5621)

This should hopefully fix the issues we have been seeing on CI. While I’m not super keen on including non-upstreamable patches it seems better than having CI be flaky. changelog_begin changelog_end
FlashSheridan · Apr 20, 2020 · db37e4c · db37e4c
1 parent 0c97fa4
commit db37e4c
Show file tree

Hide file tree

Showing 3 changed files with 95 additions and 2 deletions.
diff --git a/bazel-haskell-deps.bzl b/bazel-haskell-deps.bzl
@@ -24,6 +24,7 @@ JS_JQUERY_VERSION = "3.3.1"
 JS_DGTABLE_VERSION = "0.5.2"
 JS_FLOT_VERSION = "0.8.3"
 SHAKE_VERSION = "0.18.5"
+ZIP_VERSION = "1.5.0"
 
 def daml_haskell_deps():
     """Load all Haskell dependencies of the DAML repository."""
@@ -416,6 +417,42 @@ haskell_cabal_library(
         urls = ["http://hackage.haskell.org/package/shake-{version}/shake-{version}.tar.gz".format(version = SHAKE_VERSION)],
     )
 
+    http_archive(
+        name = "zip",
+        build_file_content = """
+load("@rules_haskell//haskell:cabal.bzl", "haskell_cabal_library")
+load("@stackage//:packages.bzl", "packages")
+haskell_cabal_library(
+    name = "zip",
+    version = "{version}",
+    srcs = glob(["**"]),
+    haddock = False,
+    deps = [
+        "@stackage//:case-insensitive",
+        "@stackage//:cereal",
+        "@stackage//:conduit",
+        "@stackage//:conduit-extra",
+        "@stackage//:digest",
+        "@stackage//:dlist",
+        "@stackage//:exceptions",
+        "@stackage//:monad-control",
+        "@stackage//:resourcet",
+        "@stackage//:transformers-base",
+    ],
+    verbose = False,
+    visibility = ["//visibility:public"],
+    flags = ["disable-bzip2"],
+)
+""".format(version = ZIP_VERSION),
+        patch_args = ["-p1"],
+        patches = [
+            "@com_github_digital_asset_daml//bazel_tools:haskell-zip.patch",
+        ],
+        sha256 = "051e891d6a13774f1d06b0251e9a0bf92f05175da8189d936c7d29c317709802",
+        strip_prefix = "zip-{}".format(ZIP_VERSION),
+        urls = ["http://hackage.haskell.org/package/zip-{version}/zip-{version}.tar.gz".format(version = ZIP_VERSION)],
+    )
+
     #
     # Stack binary
     #
@@ -510,6 +547,7 @@ exports_files(["stack.exe"], visibility = ["//visibility:public"])
             "data-default",
             "Decimal",
             "deepseq",
+            "digest",
             "directory",
             "dlist",
             "either",
@@ -639,7 +677,6 @@ exports_files(["stack.exe"], visibility = ["//visibility:public"])
             "xml",
             "xml-conduit",
             "yaml",
-            "zip",
             "zip-archive",
             "zlib",
             "zlib-bindings",
@@ -657,5 +694,6 @@ exports_files(["stack.exe"], visibility = ["//visibility:public"])
             "js-dgtable": "@js_dgtable//:js-dgtable",
             "js-flot": "@js_flot//:js-flot",
             "shake": "@shake//:shake",
+            "zip": "@zip//:zip",
         },
     )
diff --git a/bazel_tools/haskell-zip.patch b/bazel_tools/haskell-zip.patch
@@ -0,0 +1,56 @@
+The use of temp files in `zip` sadly runs into an issue
+with GHC’s `openBinaryTempFile` function which seems
+to have a race condition where multiple processes can get
+the same temp file name. Then one process will move away the temp
+file of the other process resulting in the following error:
+
+damlc.exe: C:\users\\…\\ghc77D0.zip" Just "\\\\?\\C:\\users\\…\\daml-script-1.dev.dar": does not exist (The system cannot find the file specified.)
+
+We don’t need the atomic write of the zip (DAR) file here so
+we simply patch `zip` to write to the file directly.
+It doesn’t really make sense to upstream this. The code in the
+zip library is correct, it’s `openBinaryTempFile` that is broken.
+
+diff --git a/Codec/Archive/Zip/Internal.hs b/Codec/Archive/Zip/Internal.hs
+index 674a4a3..6f68cdd 100644
+--- a/Codec/Archive/Zip/Internal.hs
++++ b/Codec/Archive/Zip/Internal.hs
+@@ -27,7 +27,6 @@ import Codec.Archive.Zip.CP437 (decodeCP437)
+ import Codec.Archive.Zip.Type
+ import Conduit (PrimMonad)
+ import Control.Applicative (many, (<|>))
+-import Control.Exception (bracketOnError, catchJust)
+ import Control.Monad
+ import Control.Monad.Catch (MonadThrow (..))
+ import Control.Monad.Trans.Maybe
+@@ -50,10 +49,7 @@ import Data.Version
+ import Data.Void
+ import Data.Word (Word16, Word32)
+ import Numeric.Natural (Natural)
+-import System.Directory
+-import System.FilePath
+ import System.IO
+-import System.IO.Error (isDoesNotExistError)
+ import qualified Data.ByteString     as B
+ import qualified Data.Conduit        as C
+ #ifdef ENABLE_BZIP2
+@@ -278,18 +274,7 @@ withNewFile
+   -> (Handle -> IO ()) -- ^ Action that writes to given 'Handle'
+   -> IO ()
+ withNewFile fpath action =
+-  bracketOnError allocate release $ \(path, h) -> do
+-    action h
+-    hClose h
+-    renameFile path fpath
+-  where
+-    allocate = openBinaryTempFile (takeDirectory fpath) ".zip"
+-    release (path, h) = do
+-      hClose h
+-      -- Despite using `bracketOnError` the file is not guaranteed to exist here
+-      -- since we could be interrupted with an async exception after the file has
+-      -- been renamed. Therefore, we silentely ignore `DoesNotExistError`.
+-      catchJust (guard . isDoesNotExistError) (removeFile path) (const $ pure ())
++    withBinaryFile fpath ReadWriteMode action
+
+ -- | Determine what comment in new archive will look like given its original
+ -- value and a collection of pending actions.
diff --git a/stack-snapshot.yaml b/stack-snapshot.yaml
@@ -28,7 +28,6 @@ packages:
   - regex-base-0.94.0.0
   - regex-tdfa-1.3.1.0
   - shake-0.18.5
-  - zip-1.3.2
   # Core packages, need to be listed for integer-simple flags.
   - integer-simple-0.1.1.1
   - text-1.2.3.1