Skip to content

Commit

Permalink
Docker: copy entire pantry directory
Browse files Browse the repository at this point in the history 
@borsboom Can you also have a look at this and see if this new logic
makes sense instead of the old index copying logic?
  • Loading branch information
@snoyberg
snoyberg committed Aug 14, 2018
1 parent 68b8a29 commit eaeae10
Showing 1 changed file with 8 additions and 19 deletions.
27 changes: 8 additions & 19 deletions src/Stack/Docker.hs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -755,25 +755,14 @@ entrypoint config@Config{..} DockerEntrypoint{..} =
unless exists $ do
ensureDir (parent destBuildPlan)
copyFile srcBuildPlan destBuildPlan
-- FIXME Manny: would it make sense to copy over the entire pantry directory?
{-
forM_ clIndices $ \pkgIdx -> do
msrcIndex <- runRIO (set stackRootL origStackRoot config) $ do
srcIndex <- configPackageIndex (indexName pkgIdx)
exists <- doesFileExist srcIndex
return $ if exists
then Just srcIndex
else Nothing
case msrcIndex of
Nothing -> return ()
Just srcIndex ->
runRIO config $ do
destIndex <- configPackageIndex (indexName pkgIdx)
exists <- doesFileExist destIndex
unless exists $ do
ensureDir (parent destIndex)
copyFile srcIndex destIndex
-}

let srcPantry = origStackRoot </> $(mkRelDir "pantry")
existsSrc <- doesDirExist srcPantry
when existsSrc $ do
runRIO config $ do
let destPantry = view stackRootL config </> $(mkRelDir "pantry")
existsDest <- doesDirExist destPantry
unless existsDest $ copyDirRecur srcPantry destPantry
return True
where
updateOrCreateStackUser estackUserEntry homeDir DockerUser{..} = do
Expand Down

3 comments on commit eaeae10

@borsboom
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know enough about pantry's storage structure to know if this makes sense. In the case of the old indices, this was pretty safe because it would only copy the index from the image if it doesn't exist yet on the host. It looks like the new code could overwrite things that already exist on the host, which is probably not desirable. A way around this might be to only copy the pantry from the image if it does not yet exist on the host.

Another thing to note is that, since we simplified the docker image generation, the images no longer contain indices (or a pantry directory), so it's unlikely this code path will ever be used.

@snoyberg
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like the new code could overwrite things that already exist on the host, which is probably not desirable.

I'm including the check if the dest exists though:

              existsDest <- doesDirExist destPantry
              unless existsDest $ copyDirRecur srcPantry destPantry

Another thing to note is that, since we simplified the docker image generation, the images no longer contain indices (or a pantry directory), so it's unlikely this code path will ever be used.

Given that, would it be OK to simply remove this code?

@borsboom
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think you can remove it.

Please sign in to comment.