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

build pkgconfig db individually when bulk fails #8496

Merged
merged 8 commits into from
Nov 5, 2022
Merged

build pkgconfig db individually when bulk fails #8496

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
dc6e136
build pkgconfig db individually when bulk fails
gbaz Sep 28, 2022
51c15d7
fix typo
gbaz Sep 30, 2022
57df0b3
Merge branch 'master' into gb/pkgconfig-fallback
gbaz Sep 30, 2022
7cb27dd
changelog, comments
gbaz Oct 21, 2022
f0c5e82
address comments
gbaz Oct 26, 2022
08bfbb6
revert unnecessary extra failure check
gbaz Oct 26, 2022
1d2409f
fix changelog
gbaz Oct 26, 2022
c42caf8
Merge branch 'master' into gb/pkgconfig-fallback
mergify[bot] Nov 5, 2022
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
25 changes: 21 additions & 4 deletions cabal-install-solver/src/Distribution/Solver/Types/PkgConfigDb.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,16 @@ import Distribution.Solver.Compat.Prelude
import Prelude ()

import Control.Exception (handle)
import Control.Monad (mapM)
import qualified Data.Map as M
import System.FilePath (splitSearchPath)

import Distribution.Compat.Environment (lookupEnv)
import Distribution.Package (PkgconfigName, mkPkgconfigName)
import Distribution.Parsec
import Distribution.Simple.Program
(ProgramDb, getProgramOutput, pkgConfigProgram, needProgram)
(ProgramDb, getProgramOutput, pkgConfigProgram, needProgram, ConfiguredProgram)
import Distribution.Simple.Program.Run (getProgramInvocationOutputAndErrors, programInvocation)
import Distribution.Simple.Utils (info)
import Distribution.Types.PkgconfigVersion
import Distribution.Types.PkgconfigVersionRange
Expand Down Expand Up @@ -65,9 +67,15 @@ readPkgConfigDb verbosity progdb = handle ioErrorHandler $ do
-- The output of @pkg-config --list-all@ also includes a description
-- for each package, which we do not need.
Copy link
Member

Choose a reason for hiding this comment

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

As witnessed by

pkg-config might be present but pkg-config --list-all might segfault. What is the strategy in such situations? (Maybe building a pkg-config database eagerly isn't so great after all, one could fill it lazily by demand in the solver, could one?)

Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like cabal treats an error in pkg-config --list-all similarly to the inability to find pkg-config, so the solver would only fail in the case where all potential solutions require at least one pkg-config package.

The solver is currently pure, so querying packages on demand would be a big change.

Copy link
Member

Choose a reason for hiding this comment

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

I acknowledge the problem that the solver would not be pure anymore, although it could be made almost pure by placing it into a service monad that only provides a method to resolve a package version via pkg-config (rather than placing it flatly into IO). Something like:

class Monad m => MonadPkgConfig m where
  pkgConfigModVersion :: PackageName -> m PackageVersion

solver :: MonadPkgConfig m => Inputs -> m Output

Lazy querying pkg-config would solve the problem where pkg-config --modversion is available but not pkg-config --list-all.

There are numerous packages out there that unconditionally declare a pkgconfig-depends (not the least, text-icu), and these all break (in certain circumstances) in the current approach with pkgconfig --list-all.

Note also that the contract given to --list-all is minimal, e.g., my man pkg-config tells me:

  --list-all
         List all modules found in the pkg-config path.

This does not specify any particular output format, something that cabal crucially relies on.

In contrast, this is the contract for --modversion:

  --modversion
         Requests  that  the  version information of the libraries specified on the command
         line be displayed.  If pkg-config can find all the libraries on the command  line,
         each  library's version string is printed to stdout, one version per line. In this
         case pkg-config exits successfully. If one or more libraries is unknown,  pkg-con-
         fig exits with a nonzero code, and the contents of stdout are undefined.

Looks rather more specific (thus, binding) to me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Programming around "an external program is segfaulting" seems like its really pushing it, though I do understand a frustration with images being broken. I would again advise that text-icu put in a flag that varies depending on pkg-config being present and working or not.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, but I'd like to know whether pkg-config --list-all works at all on macOS-11/12. Unfortunately, I cannot test it directly as I my machine is still on macOS-10. Anyone with a recent macOS willing to investigate thie?

Copy link
Member

Choose a reason for hiding this comment

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

So, upstream came back with an analysis: Homebrew/homebrew-core#112785
Not that the details matter, but the upshot is that one broken package configuration breaks pkg-config --list-all and consequently cabal-3.8.1.0.
So, install imagemagick or highway and suddently your haskell packages fail to build with cabal. Nightmare scenario? Yes!
Lesson to learn: stay away from the pkg-config --list-all path and only query pkg-config for the packages you are interested in.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Another idea is to work around the segfault by not enforcing pkg-config dependencies when pkg-config exists but the --list-all command fails. Not enforcing pkg-config dependencies is the same behavior as before #7621.

I also think that we should merge this PR first, since it already handles one case where a pkg-config command fails.

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 think a broken pkg-config is the same situation as a missing one -- a hard fail is fine, because the intended user pattern should be that there is an auto flag in the package that can switch to how to handle the case where pkg-config is unavailable (because non-existent or broken).

I am aware that not all packages have that flag that allows this switching. But we should encourage them to add it!

let pkgNames = map (takeWhile (not . isSpace)) pkgList
pkgVersions <- lines <$> getProgramOutput verbosity pkgConfig
("--modversion" : pkgNames)
(return . pkgConfigDbFromList . zip pkgNames) pkgVersions
(pkgVersions, _errs, exitCode) <-
getProgramInvocationOutputAndErrors verbosity
(programInvocation pkgConfig ("--modversion" : pkgNames))
case exitCode of
ExitSuccess -> (return . pkgConfigDbFromList . zip pkgNames) (lines pkgVersions)
-- if there's a single broken pc file the above fails, so we fall back into calling it individually
_ -> do
info verbosity ("call to pkg-config --modversion on all packages failed. Falling back to querying pkg-config individually on each package")
pkgConfigDbFromList . catMaybes <$> mapM (getIndividualVersion pkgConfig) pkgNames
where
-- For when pkg-config invocation fails (possibly because of a
-- too long command line).
Expand All @@ -80,6 +88,15 @@ readPkgConfigDb verbosity progdb = handle ioErrorHandler $ do
ioErrorHandler :: IOException -> IO PkgConfigDb
ioErrorHandler e = noPkgConfig (show e)

getIndividualVersion :: ConfiguredProgram -> String -> IO (Maybe (String, String))
getIndividualVersion pkgConfig pkg = do
gbaz marked this conversation as resolved.
Show resolved Hide resolved
(pkgVersion, _errs, exitCode) <-
getProgramInvocationOutputAndErrors verbosity
(programInvocation pkgConfig ["--modversion",pkg])
return $ case exitCode of
ExitSuccess -> Just (pkg, pkgVersion)
_ -> Nothing

-- | Create a `PkgConfigDb` from a list of @(packageName, version)@ pairs.
pkgConfigDbFromList :: [(String, String)] -> PkgConfigDb
pkgConfigDbFromList pairs = (PkgConfigDb . M.fromList . map convert) pairs
Expand Down
10 changes: 10 additions & 0 deletions changelog.d/pr-8496
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
synopsis: build pkgconfig db individually when bulk fails
packages: cabal-install cabal-install-solver
prs: #8496
issues: #8494

description: {

- When pkg-config fails to get versions for all packages in bulk, falls back to querying one-by-one.

}