Merge pull request #8623 from bairyn/fix-project-local-flags

Fix project-local flags being ignored

Author: mergify[bot]

cabal-install/src/Distribution/Client/Configure.hs

@@ -154,6 +154,9 @@ configure verbosity packageDBs repoCtxt comp platform progdb
         (fromFlagOrDefault
            (useDistPref defaultSetupScriptOptions)
            (configDistPref configFlags))
+        (fromFlagOrDefault
+            (setupConfigDynamic defaultSetupScriptOptions)
+            (configDynExe configFlags))
         (chooseCabalVersion
            configExFlags
            (flagToMaybe (configCabalVersion configExFlags)))
@@ -167,6 +170,7 @@ configureSetupScript :: PackageDBStack
                      -> Platform
                      -> ProgramDb
                      -> FilePath
+                     -> Bool
                      -> VersionRange
                      -> Maybe Lock
                      -> Bool
@@ -178,6 +182,7 @@ configureSetupScript packageDBs
                      platform
                      progdb
                      distPref
+                     dynExe
                      cabalVersion
                      lock
                      forceExternal
@@ -209,6 +214,7 @@ configureSetupScript packageDBs
     , useDependenciesExclusive = not defaultSetupDeps && isJust explicitSetupDeps
     , useVersionMacros         = not defaultSetupDeps && isJust explicitSetupDeps
     , isInteractive            = False
+    , setupConfigDynamic       = dynExe
     }
   where
     -- When we are compiling a legacy setup script without an explicit

cabal-install/src/Distribution/Client/Install.hs

@@ -1059,6 +1059,7 @@ performInstallations verbosity
         platform
         progdb
         distPref
+        (fromFlagOrDefault (setupConfigDynamic defaultSetupScriptOptions) $ configDynExe configFlags)
         (chooseCabalVersion configExFlags (libVersion miscOptions))
         (Just lock)
         parallelInstall

cabal-install/src/Distribution/Client/ProjectPlanOutput.hs

@@ -272,9 +272,9 @@ encodePlanAsJson distDirLayout elaboratedInstallPlan elaboratedSharedConfig =
     comp2str = prettyShow
 
     style2str :: Bool -> BuildStyle -> String
+    style2str _     BuildAndInstall  = "global"
     style2str True  _                = "local"
     style2str False BuildInplaceOnly = "inplace"
-    style2str False BuildAndInstall  = "global"
 
     jdisplay :: Pretty a => a -> J.Value
     jdisplay = J.String . prettyShow

cabal-install/src/Distribution/Client/ProjectPlanning.hs

@@ -668,6 +668,7 @@ rebuildInstallPlan verbosity
                          projectConfigAllPackages,
                          projectConfigLocalPackages,
                          projectConfigSpecificPackage,
+                         projectPackagesNamed,
                          projectConfigBuildOnly
                        }
                        (compiler, platform, progdb) pkgConfigDB
@@ -692,6 +693,7 @@ rebuildInstallPlan verbosity
                 localPackages
                 sourcePackageHashes
                 defaultInstallDirs
+                projectPackagesNamed
                 projectConfigShared
                 projectConfigAllPackages
                 projectConfigLocalPackages
@@ -1350,6 +1352,7 @@ elaborateInstallPlan
   -> [PackageSpecifier (SourcePackage (PackageLocation loc))]
   -> Map PackageId PackageSourceHash
   -> InstallDirs.InstallDirTemplates
+  -> [PackageVersionConstraint]
   -> ProjectConfigShared
   -> PackageConfig
   -> PackageConfig
@@ -1361,6 +1364,7 @@ elaborateInstallPlan verbosity platform compiler compilerprogdb pkgConfigDB
                      solverPlan localPackages
                      sourcePackageHashes
                      defaultInstallDirs
+                     extraPackages
                      sharedPackageConfig
                      allPackagesConfig
                      localPackagesConfig
@@ -2031,15 +2035,21 @@ elaborateInstallPlan verbosity platform compiler compilerprogdb pkgConfigDB
       $ map packageId
       $ SolverInstallPlan.reverseDependencyClosure
           solverPlan
-          (map PlannedId (Set.toList pkgsLocalToProject))
+          (map PlannedId (Set.toList pkgsInplaceToProject))
 
     isLocalToProject :: Package pkg => pkg -> Bool
     isLocalToProject pkg = Set.member (packageId pkg)
                                       pkgsLocalToProject
 
+    pkgsInplaceToProject :: Set PackageId
+    pkgsInplaceToProject =
+        Set.fromList (catMaybes (map shouldBeLocal localPackages))
+        --TODO: localPackages is a misnomer, it's all project packages
+        -- here is where we decide which ones will be local!
+
     pkgsLocalToProject :: Set PackageId
     pkgsLocalToProject =
-        Set.fromList (catMaybes (map shouldBeLocal localPackages))
+        Set.fromList (catMaybes (map (isInLocal extraPackages) localPackages))
         --TODO: localPackages is a misnomer, it's all project packages
         -- here is where we decide which ones will be local!
 
@@ -2108,6 +2118,28 @@ shouldBeLocal (SpecificSourcePackage pkg) = case srcpkgSource pkg of
     LocalUnpackedPackage _ -> Just (packageId pkg)
     _                      -> Nothing
 
+-- Used to determine which packages are affected by local package configuration
+-- flags like ‘--enable-shared --enable-executable-dynamic --disable-library-vanilla’.
+isInLocal :: [PackageVersionConstraint] -> PackageSpecifier (SourcePackage (PackageLocation loc)) -> Maybe PackageId
+isInLocal _              NamedPackage{}              = Nothing
+isInLocal _extraPackages (SpecificSourcePackage pkg) = case srcpkgSource pkg of
+    LocalUnpackedPackage _ -> Just (packageId pkg)
+    -- LocalTarballPackage is matched here too, because otherwise ‘sdistize’
+    -- produces for ‘localPackages’ in the ‘ProjectBaseContext’ a
+    -- LocalTarballPackage, and ‘shouldBeLocal’ will make flags like
+    -- ‘--disable-library-vanilla’ have no effect for a typical
+    -- ‘cabal install --lib --enable-shared enable-executable-dynamic --disable-library-vanilla’,
+    -- as these flags would apply to local packages, but the sdist would
+    -- erroneously not get categorized as a local package, so the flags would be
+    -- ignored and produce a package with an unchanged hash.
+    LocalTarballPackage  _ -> Just (packageId pkg)
+    -- TODO: the docs say ‘extra-packages’ is implemented in cabal project
+    -- files.  We can fix that here by checking that the version range matches.
+    --RemoteTarballPackage    _ -> _
+    --RepoTarballPackage      _ -> _
+    --RemoteSourceRepoPackage _ -> _
+    _                      -> Nothing
+
 -- | Given a 'ElaboratedPlanPackage', report if it matches a 'ComponentName'.
 matchPlanPkg :: (ComponentName -> Bool) -> ElaboratedPlanPackage -> Bool
 matchPlanPkg p = InstallPlan.foldPlanPackage (p . ipiComponentName) (matchElabPkg p)
@@ -3387,7 +3419,8 @@ setupHsScriptOptions (ReadyPackage elab@ElaboratedConfiguredPackage{..})
       useWin32CleanHack        = False,   --TODO: [required eventually]
       forceExternalSetupMethod = isParallelBuild,
       setupCacheLock           = Just cacheLock,
-      isInteractive            = False
+      isInteractive            = False,
+      setupConfigDynamic       = elabDynExe
     }
 
 

cabal-install/src/Distribution/Client/SetupWrapper.hs

@@ -71,7 +71,7 @@ import Distribution.Simple.BuildPaths
 import Distribution.Simple.Command
          ( CommandUI(..), commandShowOptions )
 import Distribution.Simple.Program.GHC
-         ( GhcMode(..), GhcOptions(..), renderGhcOptions )
+         ( GhcMode(..), GhcDynLinkMode(..), GhcOptions(..), renderGhcOptions )
 import qualified Distribution.Simple.PackageIndex as PackageIndex
 import Distribution.Simple.PackageIndex (InstalledPackageIndex)
 import qualified Distribution.InstalledPackageInfo as IPI
@@ -249,7 +249,12 @@ data SetupScriptOptions = SetupScriptOptions {
     -- | Is the task we are going to run an interactive foreground task,
     -- or an non-interactive background task? Based on this flag we
     -- decide whether or not to delegate ctrl+c to the spawned task
-    isInteractive            :: Bool
+    isInteractive            :: Bool,
+
+    -- Also track build output artifact configuration.
+
+    -- | Pass `-dynamic` to `ghc` for dynamic rather than static linking.
+    setupConfigDynamic       :: Bool
   }
 
 defaultSetupScriptOptions :: SetupScriptOptions
@@ -272,7 +277,8 @@ defaultSetupScriptOptions = SetupScriptOptions {
     useWin32CleanHack        = False,
     forceExternalSetupMethod = False,
     setupCacheLock           = Nothing,
-    isInteractive            = False
+    isInteractive            = False,
+    setupConfigDynamic       = False
   }
 
 workingDir :: SetupScriptOptions -> FilePath
@@ -840,6 +846,9 @@ getExternalSetupMethod verbosity options pkg bt = do
               -- --ghc-option=-v instead!
               ghcOptVerbosity       = Flag (min verbosity normal)
             , ghcOptMode            = Flag GhcModeMake
+            , ghcOptDynLinkMode     = case setupConfigDynamic options'' of
+                                      True  -> Flag GhcDynamicOnly
+                                      False -> Flag GhcStaticOnly
             , ghcOptInputFiles      = toNubListR [setupHs]
             , ghcOptOutputFile      = Flag setupProgFile
             , ghcOptObjDir          = Flag setupDir

cabal-install/tests/IntegrationTests2.hs

@@ -1591,7 +1591,7 @@ testProgramOptionsLocal config0 = do
                 (Just [ghcFlag])
                 (getProgArgs localPackages "q")
     assertEqual "p"
-                Nothing
+                (Just [ghcFlag])
                 (getProgArgs localPackages "p")
   where
     testdir = "regression/program-options"

cabal-testsuite/PackageTests/LinkerOptions/NonignoredConfigs/basic/basic/Basic.hs

@@ -0,0 +1,10 @@
+module Basic where
+
+funcs :: (a -> b -> c) -> ((a -> b -> c) -> a -> b -> c) -> b -> a -> c
+funcs f g = \a b -> (g f) b a
+
+name :: String
+name = "Basic"
+
+number :: Integer
+number = 8

cabal-testsuite/PackageTests/LinkerOptions/NonignoredConfigs/basic/basic/basic.cabal

@@ -0,0 +1,10 @@
+cabal-version: >= 1.10
+name: basic
+version: 0.1
+build-type: Simple
+
+library
+  default-language: Haskell2010
+  build-depends: base
+  exposed-modules:
+    Basic

cabal-testsuite/PackageTests/LinkerOptions/NonignoredConfigs/basic/cabal.project

@@ -0,0 +1 @@
+packages: basic

cabal-testsuite/PackageTests/LinkerOptions/NonignoredConfigs/cabal.out

@@ -0,0 +1,26 @@
+# cabal v2-install
+Wrote tarball sdist to <ROOT>/cabal.dist/work/./basic/../dist/sdist/basic-0.1.tar.gz
+Resolving dependencies...
+Build profile: -w ghc-<GHCVER> -O1
+In order, the following will be built:
+ - basic-0.1 (lib) (requires build)
+Configuring library for basic-0.1..
+Preprocessing library for basic-0.1..
+Building library for basic-0.1..
+Installing library in <PATH>
+# cabal v2-install
+Wrote tarball sdist to <ROOT>/cabal.dist/work/./basic/../dist/sdist/basic-0.1.tar.gz
+Resolving dependencies...
+Build profile: -w ghc-<GHCVER> -O1
+In order, the following will be built:
+ - basic-0.1 (lib) (requires build)
+Configuring library for basic-0.1..
+Preprocessing library for basic-0.1..
+Building library for basic-0.1..
+Installing library in <PATH>
+# cabal v2-install
+Wrote tarball sdist to <ROOT>/cabal.dist/work/./basic/../dist/sdist/basic-0.1.tar.gz
+Resolving dependencies...
+# cabal v2-install
+Wrote tarball sdist to <ROOT>/cabal.dist/work/./basic/../dist/sdist/basic-0.1.tar.gz
+Resolving dependencies...

cabal-testsuite/PackageTests/LinkerOptions/NonignoredConfigs/cabal.test.hs

@@ -0,0 +1,105 @@
+import Test.Cabal.Prelude
+
+-- This test ensures the following fix holds:
+-- > Fix project-local build flags being ignored.
+-- >
+-- > I noticed that running ‘cabal install’ with two separate sets of dynamic /
+-- > static build flags (e.g. one with none, and one with ‘--enable-shared
+-- > --enable-executable-dynamic --disable-library-vanilla’) produced packages with
+-- > the same hash, instead of different hashes.
+-- >
+-- > After debugging this issue I found that this command (with no explicit cabal
+-- > project file) was resulting in these build configuration flags being ignored,
+-- > because in ProjectPlanning.hs, the sdist was not considered a local package, so
+-- > the (non-shared) local-package-only configuration was being dropped.
+-- >
+-- > This fix ensures that these command-line arguments properly make it through to
+-- > where they belong in cases like this.
+--
+-- Basically, take a simple package, build it under two sets of build flags:
+-- > (nothing)
+-- > --enable-shared --enable-executable-dynamic --disable-library-vanilla
+--
+-- And ensure that whereas before they produced the same hash, now the package
+-- hashes produced are different.  (And also supplementarily ensure that
+-- re-running the same build with the same flags a second time produces a
+-- deterministic hash too; the hash should differ only when we change these
+-- flags.)
+--
+-- Based on the UniqueIPID test.
+
+import Control.Monad (forM, foldM_)
+import Data.List (isPrefixOf, tails)
+
+data Linking = Static | Dynamic deriving (Eq, Ord, Show)
+
+links :: [Linking]
+links = [Static, Dynamic]
+
+linkConfigFlags :: Linking -> [String]
+linkConfigFlags Static  =
+    [
+    ]
+linkConfigFlags Dynamic =
+    [
+        "--enable-shared",
+        "--enable-executable-dynamic",
+        "--disable-library-vanilla"
+    ]
+
+lrun :: [Linking]
+lrun = [Static, Dynamic, Static, Dynamic]
+
+main = cabalTest $ do
+    -- Skip if on Windows, since my default Chocolatey Windows setup (and the CI
+    -- server setup at the time, presumably) lacks support for dynamic builds
+    -- since the base package appears to be static only, lacking e.g. ‘.dyn_o’
+    -- files.  Normal Windows installations would need suport for dynamic
+    -- builds, or else this test would fail when it tries to build with the
+    -- dynamic flags.
+    skipIfWindows
+
+    withPackageDb $ do
+        -- Phase 1: get 4 hashes according to config flags.
+        results <- forM (zip [0..] lrun) $ \(idx, linking) -> do
+            withDirectory "basic" $ do
+                withSourceCopyDir ("basic" ++ show idx) $ do
+                    cwd <- fmap testSourceCopyDir getTestEnv
+                    -- (Now do ‘cd ..’, since withSourceCopyDir made our previous
+                    -- previous such withDirectories now accumulate to be
+                    -- relative to setup.dist/basic0, not testSourceDir
+                    -- (see 'testCurrentDir').)
+                    withDirectory ".." $ do
+                        packageEnv <- (</> ("basic" ++ show idx ++ ".env")) . testWorkDir <$> getTestEnv
+                        cabal "v2-install" $ ["--disable-deterministic", "--lib", "--package-env=" ++ packageEnv] ++ linkConfigFlags linking ++ ["basic"]
+                        let exIPID s = takeWhile (/= '\n') . head . filter (\t -> any (`isPrefixOf` t) ["basic-0.1-", "bsc-0.1-"]) $ tails s
+                        hashedIpid <- exIPID <$> liftIO (readFile packageEnv)
+                        return $ ((idx, linking), hashedIpid)
+        -- Phase 2: make sure we have different hashes iff we have different config flags.
+        -- In particular make sure the dynamic config flags weren't silently
+        -- dropped and ignored, since this is the bug that prompted this test.
+        (\step -> foldM_ step (const $ return ()) results) $ \acc x -> do
+            acc x
+            return $ \future -> acc future >> do
+                let
+                    ((thisIdx,   thisLinking),   thisHashedIpid)   = x
+                    ((futureIdx, futureLinking), futureHashedIpid) = future
+                when ((thisHashedIpid == futureHashedIpid) /= (thisLinking == futureLinking)) $ do
+                    assertFailure . unlines $
+                        if thisLinking /= futureLinking
+                            then
+                                -- What we are primarily concerned with testing
+                                -- here.
+                                [
+                                    "Error: static and dynamic config flags produced an IPID with the same hash; were the dynamic flags silently dropped?",
+                                    "\thashed IPID: " ++ thisHashedIpid
+                                ]
+                            else
+                                -- Help test our test can also make equal
+                                -- hashes.
+                                [
+                                    "Error: config flags were equal, yet a different IPID hash was produced.",
+                                    "\thashed IPID 1 : " ++ thisHashedIpid,
+                                    "\thashed IPID 2 : " ++ futureHashedIpid,
+                                    "\tlinking flags : " ++ show thisLinking
+                                ]

changelog.d/pr-8623

@@ -0,0 +1,7 @@
+synopsis: Fix project-local flags being ignored
+packages: cabal-install
+prs: #8623
+description: {
+    Fix some cases of configuration flags being dropped, e.g. with `v2-install`
+    and `--enable-shared --enable-executable-dynamic --disable-library-vanilla`.
+}