From 96a78beb75ba1c40b7ece2ebe0b111057c3f34bb Mon Sep 17 00:00:00 2001 From: Fabrizio Ferrai Date: Sat, 28 Sep 2024 14:18:34 +0300 Subject: [PATCH 1/8] Prevent double cloning of git repos when fetching them in parallel --- src/Spago/Command/Fetch.purs | 20 +++++++++- .../expected-stderr/lockfile-up-to-date.txt | 17 ++++++++ test/Spago/Build/Monorepo.purs | 39 ++++++++++++------- 3 files changed, 61 insertions(+), 15 deletions(-) create mode 100644 test-fixtures/monorepo/1208-no-double-cloning/expected-stderr/lockfile-up-to-date.txt diff --git a/src/Spago/Command/Fetch.purs b/src/Spago/Command/Fetch.purs index 29c2f3bb0..4dacf534a 100644 --- a/src/Spago/Command/Fetch.purs +++ b/src/Spago/Command/Fetch.purs @@ -27,12 +27,14 @@ import Data.Either as Either import Data.FunctorWithIndex (mapWithIndex) import Data.HTTP.Method as Method import Data.Int as Int +import Data.List as List import Data.Map as Map import Data.Newtype (wrap) import Data.Set as Set import Data.String (joinWith) import Data.Traversable (sequence) import Effect.Aff as Aff +import Effect.Aff.AVar as AVar import Effect.Ref as Ref import Node.Buffer as Buffer import Node.Encoding as Encoding @@ -204,11 +206,27 @@ run { packages: packagesRequestedToInstall, ensureRanges, isTest, isRepl } = do fetchPackagesToLocalCache :: ∀ a. Map PackageName Package -> Spago (FetchEnv a) Unit fetchPackagesToLocalCache packages = do { offline } <- ask + -- Before starting to fetch packages we build a Map of AVars to act as locks for each git location. + -- This is so we don't have two threads trying to clone the same repo at the same time. + gitLocks <- liftAff $ map (Map.fromFoldable <<< List.catMaybes) $ for (Map.values packages) case _ of + GitPackage gitPackage -> (Just <<< Tuple gitPackage.git) <$> AVar.new unit + _ -> pure Nothing parallelise $ packages # Map.toUnfoldable <#> \(Tuple name package) -> do let localPackageLocation = Config.getPackageLocation name package -- first of all, we check if we have the package in the local cache. If so, we don't even do the work unlessM (FS.exists localPackageLocation) case package of - GitPackage gitPackage -> getGitPackageInLocalCache name gitPackage + GitPackage gitPackage -> do + -- for git repos it's a little more involved since cloning them takes a while and we risk race conditions + -- and possibly cloning the same repo multiple times - so we use a lock on the git url to prevent that + case Map.lookup gitPackage.git gitLocks of + Nothing -> do + -- This is not supposed to happen but we just move on if it does + getGitPackageInLocalCache name gitPackage + Just lock -> do + -- Take the lock, do the git thing, release the lock + liftAff $ AVar.take lock + getGitPackageInLocalCache name gitPackage + liftAff $ AVar.put unit lock RegistryVersion v -> do -- if the version comes from the registry then we have a longer list of things to do let versionString = Registry.Version.print v diff --git a/test-fixtures/monorepo/1208-no-double-cloning/expected-stderr/lockfile-up-to-date.txt b/test-fixtures/monorepo/1208-no-double-cloning/expected-stderr/lockfile-up-to-date.txt new file mode 100644 index 000000000..8a7c9f69c --- /dev/null +++ b/test-fixtures/monorepo/1208-no-double-cloning/expected-stderr/lockfile-up-to-date.txt @@ -0,0 +1,17 @@ +Reading Spago workspace configuration... + +✓ Selecting package to build: consumer + +Downloading dependencies... +Cloning +Building... +purs compile... +purs compile... +purs compile... +purs compile... +purs compile... + Src Lib All +Warnings 0 0 0 +Errors 0 0 0 + +✓ Build succeeded. diff --git a/test/Spago/Build/Monorepo.purs b/test/Spago/Build/Monorepo.purs index d7115c949..0d9685013 100644 --- a/test/Spago/Build/Monorepo.purs +++ b/test/Spago/Build/Monorepo.purs @@ -5,6 +5,8 @@ import Test.Prelude import Data.Array as Array import Data.String (Pattern(..)) import Data.String as String +import Data.String.Regex as Regex +import Data.String.Regex.Flags as Regex.Flags import Effect.Aff (bracket) import Node.Path as Path import Node.Process as Process @@ -254,19 +256,20 @@ spec = Spec.describe "monorepo" do Spec.it "#1208: clones a monorepo only once, even if multiple packages from it are needed" \{ spago, fixture, testCwd } -> do -- A local file system Git repo to use as a remote for Spago to clone from - let createLibraryRepo = do - let libRepo = Path.concat [ Paths.paths.temp, "spago-1208" ] - whenM (FS.exists libRepo) $ rmRf libRepo - FS.copyTree { src: fixture "monorepo/1208-no-double-cloning/library", dst: libRepo } - git_ libRepo [ "init" ] - git_ libRepo [ "add", "." ] - git_ libRepo [ "config", "--global", "core.longpaths", "true" ] - git_ libRepo [ "config", "user.name", "test-user" ] - git_ libRepo [ "config", "user.email", "test-user@aol.com" ] - git_ libRepo [ "commit", "-m", "Initial commit" ] - git_ libRepo [ "tag", "v1" ] - git_ libRepo [ "tag", "v2" ] - pure libRepo + let + createLibraryRepo = do + let libRepo = Path.concat [ Paths.paths.temp, "spago-1208" ] + whenM (FS.exists libRepo) $ rmRf libRepo + FS.copyTree { src: fixture "monorepo/1208-no-double-cloning/library", dst: libRepo } + git_ libRepo [ "init" ] + git_ libRepo [ "add", "." ] + git_ libRepo [ "config", "--global", "core.longpaths", "true" ] + git_ libRepo [ "config", "user.name", "test-user" ] + git_ libRepo [ "config", "user.email", "test-user@aol.com" ] + git_ libRepo [ "commit", "-m", "Initial commit" ] + git_ libRepo [ "tag", "v1" ] + git_ libRepo [ "tag", "v2" ] + pure libRepo bracket createLibraryRepo rmRf \libRepo -> do let @@ -302,7 +305,9 @@ spec = Spec.describe "monorepo" do { stdoutFile: Nothing , stderrFile: Just $ fixture expectedFixture , result - , sanitize: String.trim >>> String.replaceAll (String.Pattern libRepo) (String.Replacement "") + , sanitize: String.trim + >>> String.replaceAll (String.Pattern libRepo) (String.Replacement "") + >>> Regex.replace (unsafeFromRight $ Regex.regex "^purs compile: .*$" (Regex.Flags.global <> Regex.Flags.multiline)) "purs compile..." } -- First run `spago install` to make sure global cache is populated, @@ -348,6 +353,12 @@ spec = Spec.describe "monorepo" do shouldBeSuccessErr' "monorepo/1208-no-double-cloning/expected-stderr/four-deps.txt" assertRefCheckedOut "lib4" "v4" + -- Lockfile test: when it's up to date but the cache is not populated (i.e. a fresh clone) + -- then there are no double clones. This is a regression test for #1206 + spago [ "build" ] >>= shouldBeSuccess + rmRf ".spago" + spago [ "build" ] >>= shouldBeSuccessErr' "monorepo/1208-no-double-cloning/expected-stderr/lockfile-up-to-date.txt" + where git_ cwd = void <<< git cwd From caa7dc0dc9fd0f16270015259b0fedee0ead41ac Mon Sep 17 00:00:00 2001 From: Fabrizio Ferrai Date: Sat, 28 Sep 2024 18:17:02 +0300 Subject: [PATCH 2/8] trim last --- test/Spago/Build/Monorepo.purs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/Spago/Build/Monorepo.purs b/test/Spago/Build/Monorepo.purs index 0d9685013..03ff254b3 100644 --- a/test/Spago/Build/Monorepo.purs +++ b/test/Spago/Build/Monorepo.purs @@ -305,9 +305,10 @@ spec = Spec.describe "monorepo" do { stdoutFile: Nothing , stderrFile: Just $ fixture expectedFixture , result - , sanitize: String.trim - >>> String.replaceAll (String.Pattern libRepo) (String.Replacement "") - >>> Regex.replace (unsafeFromRight $ Regex.regex "^purs compile: .*$" (Regex.Flags.global <> Regex.Flags.multiline)) "purs compile..." + , sanitize: + String.replaceAll (String.Pattern libRepo) (String.Replacement "") + >>> Regex.replace (unsafeFromRight $ Regex.regex "^purs compile: .*$" (Regex.Flags.global <> Regex.Flags.multiline)) "purs compile..." + >>> String.trim } -- First run `spago install` to make sure global cache is populated, From 70dfb53edccdeba20fc2dbece66fdda98b42dd77 Mon Sep 17 00:00:00 2001 From: Fabrizio Ferrai Date: Sat, 28 Sep 2024 20:29:07 +0300 Subject: [PATCH 3/8] Maybe stripping half of the fixture helps --- .../expected-stderr/lockfile-up-to-date.txt | 10 ---------- test/Spago/Build/Monorepo.purs | 2 +- 2 files changed, 1 insertion(+), 11 deletions(-) diff --git a/test-fixtures/monorepo/1208-no-double-cloning/expected-stderr/lockfile-up-to-date.txt b/test-fixtures/monorepo/1208-no-double-cloning/expected-stderr/lockfile-up-to-date.txt index 8a7c9f69c..3ce0c0435 100644 --- a/test-fixtures/monorepo/1208-no-double-cloning/expected-stderr/lockfile-up-to-date.txt +++ b/test-fixtures/monorepo/1208-no-double-cloning/expected-stderr/lockfile-up-to-date.txt @@ -5,13 +5,3 @@ Reading Spago workspace configuration... Downloading dependencies... Cloning Building... -purs compile... -purs compile... -purs compile... -purs compile... -purs compile... - Src Lib All -Warnings 0 0 0 -Errors 0 0 0 - -✓ Build succeeded. diff --git a/test/Spago/Build/Monorepo.purs b/test/Spago/Build/Monorepo.purs index 03ff254b3..59de240ae 100644 --- a/test/Spago/Build/Monorepo.purs +++ b/test/Spago/Build/Monorepo.purs @@ -307,7 +307,7 @@ spec = Spec.describe "monorepo" do , result , sanitize: String.replaceAll (String.Pattern libRepo) (String.Replacement "") - >>> Regex.replace (unsafeFromRight $ Regex.regex "^purs compile: .*$" (Regex.Flags.global <> Regex.Flags.multiline)) "purs compile..." + >>> Regex.replace (unsafeFromRight $ Regex.regex "Building...[\\s\\S]*" (Regex.Flags.global <> Regex.Flags.multiline)) "Building..." >>> String.trim } From 44890617551ec14539d4a3f3f813a7aba7df6ecc Mon Sep 17 00:00:00 2001 From: Fyodor Soikin Date: Sat, 28 Sep 2024 14:57:16 -0400 Subject: [PATCH 4/8] Restore regex --- test/Spago/Build/Monorepo.purs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/Spago/Build/Monorepo.purs b/test/Spago/Build/Monorepo.purs index 59de240ae..03ff254b3 100644 --- a/test/Spago/Build/Monorepo.purs +++ b/test/Spago/Build/Monorepo.purs @@ -307,7 +307,7 @@ spec = Spec.describe "monorepo" do , result , sanitize: String.replaceAll (String.Pattern libRepo) (String.Replacement "") - >>> Regex.replace (unsafeFromRight $ Regex.regex "Building...[\\s\\S]*" (Regex.Flags.global <> Regex.Flags.multiline)) "Building..." + >>> Regex.replace (unsafeFromRight $ Regex.regex "^purs compile: .*$" (Regex.Flags.global <> Regex.Flags.multiline)) "purs compile..." >>> String.trim } From db64008ec8af9fd95748d69b0132dc85707e5283 Mon Sep 17 00:00:00 2001 From: Fyodor Soikin Date: Sat, 28 Sep 2024 14:58:28 -0400 Subject: [PATCH 5/8] Update fixture --- .../expected-stderr/lockfile-up-to-date.txt | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/test-fixtures/monorepo/1208-no-double-cloning/expected-stderr/lockfile-up-to-date.txt b/test-fixtures/monorepo/1208-no-double-cloning/expected-stderr/lockfile-up-to-date.txt index 3ce0c0435..8a7c9f69c 100644 --- a/test-fixtures/monorepo/1208-no-double-cloning/expected-stderr/lockfile-up-to-date.txt +++ b/test-fixtures/monorepo/1208-no-double-cloning/expected-stderr/lockfile-up-to-date.txt @@ -5,3 +5,13 @@ Reading Spago workspace configuration... Downloading dependencies... Cloning Building... +purs compile... +purs compile... +purs compile... +purs compile... +purs compile... + Src Lib All +Warnings 0 0 0 +Errors 0 0 0 + +✓ Build succeeded. From cfc28f00247a32b267f6c02bf1c1cf7ada2d061e Mon Sep 17 00:00:00 2001 From: Fyodor Soikin Date: Sat, 28 Sep 2024 15:00:00 -0400 Subject: [PATCH 6/8] Newlines --- test/Spago/Build/Monorepo.purs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/Spago/Build/Monorepo.purs b/test/Spago/Build/Monorepo.purs index 03ff254b3..01828c011 100644 --- a/test/Spago/Build/Monorepo.purs +++ b/test/Spago/Build/Monorepo.purs @@ -306,7 +306,8 @@ spec = Spec.describe "monorepo" do , stderrFile: Just $ fixture expectedFixture , result , sanitize: - String.replaceAll (String.Pattern libRepo) (String.Replacement "") + String.replaceAll (String.Pattern "\r\n") (String.Replacement "\n") + >>> String.replaceAll (String.Pattern libRepo) (String.Replacement "") >>> Regex.replace (unsafeFromRight $ Regex.regex "^purs compile: .*$" (Regex.Flags.global <> Regex.Flags.multiline)) "purs compile..." >>> String.trim } From df0a144aa1a26535202159f528db43240ac7e01c Mon Sep 17 00:00:00 2001 From: Fabrizio Ferrai Date: Sun, 29 Sep 2024 00:27:04 +0300 Subject: [PATCH 7/8] It's always traverse Co-authored-by: Fyodor Soikin --- src/Spago/Command/Fetch.purs | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/Spago/Command/Fetch.purs b/src/Spago/Command/Fetch.purs index 4dacf534a..0e35fe9a9 100644 --- a/src/Spago/Command/Fetch.purs +++ b/src/Spago/Command/Fetch.purs @@ -218,15 +218,11 @@ fetchPackagesToLocalCache packages = do GitPackage gitPackage -> do -- for git repos it's a little more involved since cloning them takes a while and we risk race conditions -- and possibly cloning the same repo multiple times - so we use a lock on the git url to prevent that - case Map.lookup gitPackage.git gitLocks of - Nothing -> do - -- This is not supposed to happen but we just move on if it does - getGitPackageInLocalCache name gitPackage - Just lock -> do - -- Take the lock, do the git thing, release the lock - liftAff $ AVar.take lock - getGitPackageInLocalCache name gitPackage - liftAff $ AVar.put unit lock + let lock = Map.lookup gitPackage.git gitLocks + -- Take the lock, do the git thing, release the lock + liftAff $ AVar.take `traverse_` lock + getGitPackageInLocalCache name gitPackage + liftAff $ AVar.put unit `traverse_` lock RegistryVersion v -> do -- if the version comes from the registry then we have a longer list of things to do let versionString = Registry.Version.print v From 6a17e3a9dedc255cd71beb8967fa591061eea908 Mon Sep 17 00:00:00 2001 From: Fabrizio Ferrai Date: Sun, 29 Sep 2024 00:30:15 +0300 Subject: [PATCH 8/8] Import traverse --- src/Spago/Command/Fetch.purs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Spago/Command/Fetch.purs b/src/Spago/Command/Fetch.purs index 0e35fe9a9..704f41adf 100644 --- a/src/Spago/Command/Fetch.purs +++ b/src/Spago/Command/Fetch.purs @@ -32,7 +32,7 @@ import Data.Map as Map import Data.Newtype (wrap) import Data.Set as Set import Data.String (joinWith) -import Data.Traversable (sequence) +import Data.Traversable (sequence, traverse_) import Effect.Aff as Aff import Effect.Aff.AVar as AVar import Effect.Ref as Ref