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

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

Merged
merged 7 commits into from May 7, 2023

Conversation

gbaz
Copy link
Collaborator

@gbaz gbaz commented Apr 12, 2023

Resolves #8894

Auto adds base to cabal env files generated by cabal install --lib

Also resolves #8825 by checking that filepaths of specificpackagedbs exist before adding them to the env file.

Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

Changes look good to me, but a couple of lines of documentation would be great.

Comment on lines +726 to +727
globalPackages :: [PackageName]
globalPackages = mkPackageName <$> [ "base" ]
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

Comment on lines +706 to +707
getLatest = (=<<) (maybeToList . safeHead . snd) . take 1 . sortBy (comparing (Down . fst))
. PI.lookupPackageName installedIndex
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.

Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +726 to +727
globalPackages :: [PackageName]
globalPackages = mkPackageName <$> [ "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.

@ulysses4ever
Copy link
Collaborator

I really wish there was a simplest test checking that in the created environment you can do basic things (like use True, as in the original bug report).

@ulysses4ever
Copy link
Collaborator

@gbaz do you know if after I: 1) call cabal install --lib blah, 2) upgrade GHC, 3) call cabal install --lib foo — I end up with two bases of different versions in the environment?

@gbaz
Copy link
Collaborator Author

gbaz commented Apr 20, 2023

@jwaldmann @gksato can you test if this resolves your issues?

@gksato
Copy link

gksato commented Apr 21, 2023

@gbaz Alrighty! I'm doing the job...

@gbaz
Copy link
Collaborator Author

gbaz commented Apr 21, 2023

@gbaz do you know if after I: 1) call cabal install --lib blah, 2) upgrade GHC, 3) call cabal install --lib foo — I end up with two bases of different versions in the environment?

@ulysses4ever what do you think the correct behavior here should be? Should it strip the older base version out?

@gksato
Copy link

gksato commented Apr 21, 2023

@gbaz

can you test if this resolves your issues?

It looks like it works on my Mac!

[EDIT] Oh, I forgot some lines. Adding them. [/EDIT]

$ cd ~/Downloads/cabal-gb-base-in-cabal-install-lib

$ cabal v2-install --installdir=~ --install-method=copy --project-file=cabal.project.release cabal-install
[omitted some lines]
Copying 'cabal' to '~/cabal'

$ cd ~
$ cp Downloads/cabal-gb-base-in-cabal-install-lib/'~'/cabal ~/cabal
$ mv ~/.cabal ~/.cabal-save
$ rm -rf ~/.ghc

$ ~/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-21T20:12:58Z.

$ ~/cabal v2-install --lib base
Resolving dependencies...

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

$ 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 "GHC's working!"
GHC's working!
ghci> :q
Leaving GHCi.

@ulysses4ever
Copy link
Collaborator

@gksato you do:

cabal v2-install --lib base

could you try some other library? Because I assume that this command worked just as well before the patch here.

@gbaz

what do you think the correct behavior here should be? Should it strip the older base version out?

I think that would be the right thing to do (strip the old one).

@gksato
Copy link

gksato commented Apr 21, 2023

@ulysses4ever

@gbaz pinged me here for #8825, which says cabal v2-install --lib base didn't work in some cases. If you would like me to try the fix for #8894, I would never mind doing that, but would you wait for me until I finish my breakfast?

@ulysses4ever
Copy link
Collaborator

@gksato ouch, I mixed up my issues here, sorry! Have a wonderful breakfast!

@gksato
Copy link

gksato commented Apr 22, 2023

@gbaz @ulysses4ever

Hey, it doesn't look like it works for #8894 😢.

$ cp Downloads/cabal-gb-base-in-cabal-install-lib/'~'/cabal ~/cabal
$ mv ~/.cabal ~/.cabal-save
$ rm -rf ~/.ghc

$ ~/cabal v2-update
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-21T22:04:15Z.
To revert to previous state run:
    cabal v2-update 'hackage.haskell.org,2023-04-21T20:12:58Z'

$ ~/cabal v2-install --lib leancheck
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)

Even in this case, the env file doesn't refer to the cabal's package database, where the library leancheck actually resides!

Copy link

@gksato gksato left a comment

Choose a reason for hiding this comment

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

This may be the cause of the failure in the comment above. If this is actually the reason for this problem, it should only occur before the creation of cabal-install's package database. I hope this helps.

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

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

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!

@gbaz gbaz requested a review from gksato May 4, 2023 20:07
Copy link

@gksato gksato left a comment

Choose a reason for hiding this comment

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

LGTM, it worked!

$ gh pr checkout 8903
[Several lines omitted]
Updating 7acef4f0a..9d85d8beb
Fast-forward
 cabal-install/src/Distribution/Client/CmdInstall.hs | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

$ ghc --version
The Glorious Glasgow Haskell Compilation System, version 9.4.5

$ cabal v2-install --installdir=$HOME --install-method=copy --project-file=cabal.project.release cabal-install --overwrite-policy=always
[Several lines omitted]
Copying 'cabal' to '/Users/gksato/cabal'

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

$ ~/cabal --version
cabal-install version 3.11
compiled using version 3.11.0.0 of the Cabal library

$ ~/cabal v2-update
[output omitted]

$ ~/cabal v2-install --lib base
Resolving dependencies...

$ cat ~/.ghc/x86_64-darwin-9.4.5/environments/default 
clear-package-db
global-package-db
package-id base-4.17.1.0

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

$ ~/cabal v2-install --lib leancheck
Resolving dependencies...
Build profile: -w ghc-9.4.5 -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.5/environments/default 
clear-package-db
global-package-db
package-db /Users/gksato/.local/state/cabal/store/ghc-9.4.5/package.db
package-id base-4.17.1.0
package-id lnchck-1.0.0-a93b8f5d

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

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

$ ~/cabal v2-update
[Output omitted]

$ ~/cabal v2-install --lib leancheck
Resolving dependencies...
Build profile: -w ghc-9.4.5 -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.5/environments/default 
clear-package-db
global-package-db
package-db /Users/gksato/.local/state/cabal/store/ghc-9.4.5/package.db
package-id base-4.17.1.0
package-id lnchck-1.0.0-a93b8f5d

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

@ulysses4ever
Copy link
Collaborator

@gbaz needs a rebase.

@gbaz
Copy link
Collaborator Author

gbaz commented May 5, 2023

can i get a second ✅ from an approved reviewer?

Copy link
Member

@Kleidukos Kleidukos left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Comment on lines +726 to +727
globalPackages :: [PackageName]
globalPackages = mkPackageName <$> [ "base" ]
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

@gbaz gbaz added the squash+merge me Tell Mergify Bot to squash-merge label May 5, 2023
@ulysses4ever
Copy link
Collaborator

How about changelog?

@gbaz
Copy link
Collaborator Author

gbaz commented May 5, 2023

good point, done!

Copy link
Collaborator

@ulysses4ever ulysses4ever left a comment

Choose a reason for hiding this comment

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

Terrific, thank you!

There's a whitespace hiccup in the last commit (in the comment).

@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label May 7, 2023
@mergify mergify bot merged commit 10de4e5 into master May 7, 2023
43 checks passed
@mergify mergify bot deleted the gb/base-in-cabal-install-lib branch May 7, 2023 23:49
@ulysses4ever
Copy link
Collaborator

@mergify backport 3.10

@mergify
Copy link
Contributor

mergify bot commented May 8, 2023

backport 3.10

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request May 8, 2023
* add base to cabal install --lib default env file

* check packagedb stack exists

* fix validdb filtering -- move later, test for a dir

* Update CmdInstall.hs

* changelog

* fix comment

---------

Co-authored-by: Gershom Bazerman <gershom@arista.com>
(cherry picked from commit 10de4e5)
mergify bot added a commit that referenced this pull request May 8, 2023
add base to cabal install --lib default env file (backport #8903)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
attention: needs-manual-qa PR is destined for manual QA attention: needs-test merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days squash+merge me Tell Mergify Bot to squash-merge
Projects
Status: Testing Approved
7 participants