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.

Sign up for GitHub

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

add base to cabal install --lib default env file #8903

Merged
merged 7 commits into from
May 7, 2023
Merged

add base to cabal install --lib default env file #8903

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e3b57e3
add base to cabal install --lib default env file
gbaz Apr 12, 2023
7acef4f
check packagedb stack exists
gbaz Apr 12, 2023
9d85d8b
fix validdb filtering -- move later, test for a dir
gbaz May 4, 2023
3fe3de3
Merge branch 'master' into gb/base-in-cabal-install-lib
gbaz May 5, 2023
428b78f
Update CmdInstall.hs
gbaz May 5, 2023
b4c8c4c
changelog
gbaz May 5, 2023
90afcd1
fix comment
gbaz May 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 21 additions & 6 deletions cabal-install/src/Distribution/Client/CmdInstall.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ import Distribution.Simple.Configure
( configCompilerEx )
import Distribution.Simple.Compiler
( Compiler(..), CompilerId(..), CompilerFlavor(..)
, PackageDBStack )
, PackageDBStack, PackageDB(..) )
import Distribution.Simple.GHC
( ghcPlatformAndVersionString, getGhcAppDir
, GhcImplInfo(..), getImplInfo
Expand All @@ -123,11 +123,13 @@ import Distribution.Verbosity
import Distribution.Simple.Utils
( wrapText, die', notice, warn
, withTempDirectory, createDirectoryIfMissingVerbose
, ordNub )
, ordNub, safeHead )
import Distribution.Utils.Generic
( writeFileAtomic )

import qualified Data.ByteString.Lazy.Char8 as BS
import Data.Ord
( Down(..) )
import qualified Data.Map as Map
import qualified Data.Set as S
import qualified Data.List.NonEmpty as NE
Expand Down Expand Up @@ -355,7 +357,10 @@ installAction flags@NixStyleFlags { extraFlags = clientInstallFlags', .. } targe
envFile <- getEnvFile clientInstallFlags platform compilerVersion
existingEnvEntries <-
getExistingEnvEntries verbosity compilerFlavor supportsPkgEnvFiles envFile
packageDbs <- getPackageDbStack compilerId projectConfigStoreDir projectConfigLogsDir
packageDbs' <- getPackageDbStack compilerId projectConfigStoreDir projectConfigLogsDir
let validDb (SpecificPackageDB fp) = doesFileExist fp
validDb _ = pure True
packageDbs <- filterM validDb packageDbs'
Copy link

@gksato gksato Apr 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Although this (filterM validDb) is the right thing to do, but this line may not be the right place to do this. Assuming that the creation of cabal-install's package database is done in the build step (lines 419--420), in the execution which creates the database, the list packageDbs holds fewer than we need it to. My speculation might be a total mistake, though, because I haven't looked deep down into the implementation of runProjectBuildPhase. Anyway, whether or not my assumption is correct, packageDbs <- filterM vaildDb packageDbs should be placed after the creation of the database.

Copy link

Choose a reason for hiding this comment

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

Also, the implementation of validDb is incorrect because a package DB is a directory (I'm not sure it is specified to be a directory, but it was one when I ran a debug). It should be:

validDb (SpecificPackageDB fp) = doesPathExist {- maybe `doesDirectoryExist` ? -} fp
validDb _ = pure True

Copy link

@gksato gksato Apr 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It looks like the two points above are actually (at least) a part of the cause of the problem. See the following shell session:

$ cd ~/Downloads/cabal/

$ git status
On branch gb/base-in-cabal-install-lib
Your branch is up to date with 'origin/gb/base-in-cabal-install-lib'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   cabal-install/src/Distribution/Client/CmdInstall.hs

no changes added to commit (use "git add" and/or "git commit -a")

$ git diff
diff --git a/cabal-install/src/Distribution/Client/CmdInstall.hs b/cabal-install/src/Distribution/Client/CmdInstall.hs
index 49b264f36..8ed288028 100644
--- a/cabal-install/src/Distribution/Client/CmdInstall.hs
+++ b/cabal-install/src/Distribution/Client/CmdInstall.hs
@@ -358,8 +358,13 @@ installAction flags@NixStyleFlags { extraFlags = clientInstallFlags', .. } targe
   existingEnvEntries <-
     getExistingEnvEntries verbosity compilerFlavor supportsPkgEnvFiles envFile
   packageDbs' <- getPackageDbStack compilerId projectConfigStoreDir projectConfigLogsDir
-  let validDb (SpecificPackageDB fp) = doesFileExist fp
-      validDb _ = pure True
+  let validDb' (SpecificPackageDB fp) = doesPathExist fp
+      validDb' _ = pure True
+      validDb db = do
+        putStrLn $ "validDb called against " ++ show db
+        res <- validDb' db
+        putStrLn $ "validDb res: " ++ show res
+        return res
   packageDbs <- filterM validDb packageDbs'
   installedIndex <- getInstalledPackages verbosity compiler packageDbs progDb
 
$ cabal v2-install --installdir=$HOME --install-method=copy --project-file=cabal.project.release cabal-install --overwrite-policy=always
Wrote tarball sdist to
/Users/gksato/Downloads/cabal/dist-newstyle/sdist/Cabal-syntax-3.11.0.0.tar.gz
Wrote tarball sdist to
/Users/gksato/Downloads/cabal/dist-newstyle/sdist/Cabal-3.11.0.0.tar.gz
Wrote tarball sdist to
/Users/gksato/Downloads/cabal/dist-newstyle/sdist/cabal-install-solver-3.11.0.0.tar.gz
Wrote tarball sdist to
/Users/gksato/Downloads/cabal/dist-newstyle/sdist/cabal-install-3.11.0.0.tar.gz
Resolving dependencies...
Up to date
Copying 'cabal' to '/Users/gksato/cabal'

$ cd ~
$ mv ~/.cabal ~/.cabal-save
$ rm -rf ~/.cabal ~/.cache/cabal ~/.cache/cabal-helper ~/.config/cabal ~/.ghc ~/.local/state/cabal

$ ~/cabal v2-update
Config file path source is default config file.
Config file not found: /Users/gksato/.config/cabal/config
Writing default configuration to /Users/gksato/.config/cabal/config
Downloading the latest package list from hackage.haskell.org
Package list of hackage.haskell.org has been updated.
The index-state is set to 2023-04-23T19:32:13Z.

$ ~/cabal v2-install --lib leancheck
validDb called against GlobalPackageDB
validDb res: True
validDb called against SpecificPackageDB "/Users/gksato/.local/state/cabal/store/ghc-9.4.4/package.db"
validDb res: False
Resolving dependencies...
Build profile: -w ghc-9.4.4 -O1
In order, the following will be built (use -v for more details):
 - leancheck-1.0.0 (lib) (requires download & build)
Downloading  leancheck-1.0.0
Downloaded   leancheck-1.0.0
Starting     leancheck-1.0.0 (lib)
Building     leancheck-1.0.0 (lib)
Installing   leancheck-1.0.0 (lib)
Completed    leancheck-1.0.0 (lib)

$ cat ~/.ghc/x86_64-darwin-9.4.4/environments/default 
clear-package-db
global-package-db
package-id base-4.17.0.0
package-id lnchck-1.0.0-b81085f1

$ ghci
Loaded package environment from /Users/gksato/.ghc/x86_64-darwin-9.4.4/environments/default
GHCi, version 9.4.4: https://www.haskell.org/ghc/  :? for help
<command line>: cannot satisfy -package-id lnchck-1.0.0-b81085f1
    (use -v for more information)

$ rm -rf ~/.ghc

$ ~/cabal v2-install --lib leancheck
validDb called against GlobalPackageDB
validDb res: True
validDb called against SpecificPackageDB "/Users/gksato/.local/state/cabal/store/ghc-9.4.4/package.db"
validDb res: True
Resolving dependencies...

$ cat ~/.ghc/x86_64-darwin-9.4.4/environments/default 
clear-package-db
global-package-db
package-db /Users/gksato/.local/state/cabal/store/ghc-9.4.4/package.db
package-id base-4.17.0.0
package-id lnchck-1.0.0-b81085f1

$ ghci
Loaded package environment from /Users/gksato/.ghc/x86_64-darwin-9.4.4/environments/default
GHCi, version 9.4.4: https://www.haskell.org/ghc/  :? for help
ghci> putStrLn "GHCi ran successfully!"
GHCi ran successfully!
ghci> :q
Leaving GHCi.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks for the careful investigation! Both issues seem legit and I'll try to do some more work in the next few days...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed both issues raised and tested to ensure they covered the problem. Thanks again!

installedIndex <- getInstalledPackages verbosity compiler packageDbs progDb

let
Expand Down Expand Up @@ -424,7 +429,7 @@ installAction flags@NixStyleFlags { extraFlags = clientInstallFlags', .. } targe
unless dryRun $
if installLibs
then installLibraries verbosity
buildCtx compiler packageDbs envFile nonGlobalEnvEntries'
buildCtx installedIndex compiler packageDbs envFile nonGlobalEnvEntries'
else installExes verbosity
baseCtx buildCtx platform compiler configFlags clientInstallFlags
where
Expand Down Expand Up @@ -687,20 +692,26 @@ installExes verbosity baseCtx buildCtx platform compiler
installLibraries
:: Verbosity
-> ProjectBuildContext
-> PI.PackageIndex InstalledPackageInfo
-> Compiler
-> PackageDBStack
-> FilePath -- ^ Environment file
-> [GhcEnvironmentFileEntry]
-> IO ()
installLibraries verbosity buildCtx compiler
installLibraries verbosity buildCtx installedIndex compiler
packageDbs envFile envEntries = do
if supportsPkgEnvFiles $ getImplInfo compiler
then do
let
getLatest = (=<<) (maybeToList . safeHead . snd) . take 1 . sortBy (comparing (Down . fst))
. PI.lookupPackageName installedIndex
Comment on lines +708 to +709
Copy link
Collaborator

Choose a reason for hiding this comment

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

This might be universally useful, how about adding this function to PackageIndex with documentation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was hoping we could backport this into the cabal 3.10 series, which last I understood, we hoped to release just cabal-install without a new Cabal library...

Copy link
Member

Choose a reason for hiding this comment

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

We'd need to have a fresh look at the planned fixes to make sure it's possible, but not releasing a new Cabal version would indeed be preferred.

globalLatest = concat (getLatest <$> globalPackages)
globalEntries = GhcEnvFilePackageId . installedUnitId <$> globalLatest
baseEntries =
GhcEnvFileClearPackageDbStack : fmap GhcEnvFilePackageDb packageDbs
pkgEntries = ordNub $
envEntries
globalEntries
++ envEntries
++ entriesForLibraryComponents (targetsMap buildCtx)
contents' = renderGhcEnvironmentFile (baseEntries ++ pkgEntries)
createDirectoryIfMissing True (takeDirectory envFile)
Expand All @@ -711,6 +722,10 @@ installLibraries verbosity buildCtx compiler
++ "so only executables will be available. (Library installation is "
++ "supported on GHC 8.0+ only)"


globalPackages :: [PackageName]
globalPackages = mkPackageName <$> [ "base" ]
Comment on lines +730 to +731
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should add here a reason why we unconditionally add "base".

Copy link
Member

Choose a reason for hiding this comment

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

And not any other built-in package or whatever the smallest enclosing set is.

Copy link
Member

Choose a reason for hiding this comment

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

follow-up in #8938


warnIfNoExes :: Verbosity -> ProjectBuildContext -> IO ()
warnIfNoExes verbosity buildCtx =
when noExes $
Expand Down