From b547ead58bf09bb838c13f02afb2f1042ad1bc7c Mon Sep 17 00:00:00 2001 From: Byron Johnson Date: Fri, 5 Aug 2022 18:33:50 -0600 Subject: [PATCH 1/2] Fix project-local build flags being ignored. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. Additionally, adjust the ‘style’ attribute in plan.json so that globally installed packages are designated as global even if they are local to the project. (Without this adjustment to ‘style2str’, the T5782Diamond test fails, because it looks up ‘dist-dirs’ in plan.json, where ‘dist-dirs’ is absent from the JSON.) Finally, take into account elabDynExe and configDynExe to provide GHC with ‘-dynamic’ appropriately rather than going about it with static linking. --- .../src/Distribution/Client/Configure.hs | 6 +++ .../src/Distribution/Client/Install.hs | 1 + .../Distribution/Client/ProjectPlanOutput.hs | 2 +- .../Distribution/Client/ProjectPlanning.hs | 39 +++++++++++++++++-- .../src/Distribution/Client/SetupWrapper.hs | 15 +++++-- cabal-install/tests/IntegrationTests2.hs | 2 +- changelog.d/pr-8623 | 7 ++++ 7 files changed, 64 insertions(+), 8 deletions(-) create mode 100644 changelog.d/pr-8623 diff --git a/cabal-install/src/Distribution/Client/Configure.hs b/cabal-install/src/Distribution/Client/Configure.hs index 2cbe16096a4..554785ff847 100644 --- a/cabal-install/src/Distribution/Client/Configure.hs +++ b/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 diff --git a/cabal-install/src/Distribution/Client/Install.hs b/cabal-install/src/Distribution/Client/Install.hs index 2baa8af9e49..a53c7ded1aa 100644 --- a/cabal-install/src/Distribution/Client/Install.hs +++ b/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 diff --git a/cabal-install/src/Distribution/Client/ProjectPlanOutput.hs b/cabal-install/src/Distribution/Client/ProjectPlanOutput.hs index c9243c310e0..fde7ea8b97a 100644 --- a/cabal-install/src/Distribution/Client/ProjectPlanOutput.hs +++ b/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 diff --git a/cabal-install/src/Distribution/Client/ProjectPlanning.hs b/cabal-install/src/Distribution/Client/ProjectPlanning.hs index 8f391c9ec99..ec437329377 100644 --- a/cabal-install/src/Distribution/Client/ProjectPlanning.hs +++ b/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 } diff --git a/cabal-install/src/Distribution/Client/SetupWrapper.hs b/cabal-install/src/Distribution/Client/SetupWrapper.hs index e4885ed07c6..239e1a37908 100644 --- a/cabal-install/src/Distribution/Client/SetupWrapper.hs +++ b/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 diff --git a/cabal-install/tests/IntegrationTests2.hs b/cabal-install/tests/IntegrationTests2.hs index 0bdf1e964a6..35e4b61b53b 100644 --- a/cabal-install/tests/IntegrationTests2.hs +++ b/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" diff --git a/changelog.d/pr-8623 b/changelog.d/pr-8623 new file mode 100644 index 00000000000..29c9c3bb2c5 --- /dev/null +++ b/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`. +} From 867cbb9544d43e43a79280f6405ecf1f6b9be4f3 Mon Sep 17 00:00:00 2001 From: Byron Johnson Date: Fri, 5 Aug 2022 22:13:42 -0600 Subject: [PATCH 2/2] =?UTF-8?q?Add=20a=20=E2=80=98NonignoredConfigs?= =?UTF-8?q?=E2=80=99=20test=20that=20fails=20without=20our=20fix.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../NonignoredConfigs/basic/basic/Basic.hs | 10 ++ .../NonignoredConfigs/basic/basic/basic.cabal | 10 ++ .../NonignoredConfigs/basic/cabal.project | 1 + .../LinkerOptions/NonignoredConfigs/cabal.out | 26 +++++ .../NonignoredConfigs/cabal.test.hs | 105 ++++++++++++++++++ 5 files changed, 152 insertions(+) create mode 100644 cabal-testsuite/PackageTests/LinkerOptions/NonignoredConfigs/basic/basic/Basic.hs create mode 100644 cabal-testsuite/PackageTests/LinkerOptions/NonignoredConfigs/basic/basic/basic.cabal create mode 100644 cabal-testsuite/PackageTests/LinkerOptions/NonignoredConfigs/basic/cabal.project create mode 100644 cabal-testsuite/PackageTests/LinkerOptions/NonignoredConfigs/cabal.out create mode 100644 cabal-testsuite/PackageTests/LinkerOptions/NonignoredConfigs/cabal.test.hs diff --git a/cabal-testsuite/PackageTests/LinkerOptions/NonignoredConfigs/basic/basic/Basic.hs b/cabal-testsuite/PackageTests/LinkerOptions/NonignoredConfigs/basic/basic/Basic.hs new file mode 100644 index 00000000000..d8690293fff --- /dev/null +++ b/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 diff --git a/cabal-testsuite/PackageTests/LinkerOptions/NonignoredConfigs/basic/basic/basic.cabal b/cabal-testsuite/PackageTests/LinkerOptions/NonignoredConfigs/basic/basic/basic.cabal new file mode 100644 index 00000000000..90f2414bc6a --- /dev/null +++ b/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 diff --git a/cabal-testsuite/PackageTests/LinkerOptions/NonignoredConfigs/basic/cabal.project b/cabal-testsuite/PackageTests/LinkerOptions/NonignoredConfigs/basic/cabal.project new file mode 100644 index 00000000000..6b9fac75bce --- /dev/null +++ b/cabal-testsuite/PackageTests/LinkerOptions/NonignoredConfigs/basic/cabal.project @@ -0,0 +1 @@ +packages: basic diff --git a/cabal-testsuite/PackageTests/LinkerOptions/NonignoredConfigs/cabal.out b/cabal-testsuite/PackageTests/LinkerOptions/NonignoredConfigs/cabal.out new file mode 100644 index 00000000000..fa5cb34ec0e --- /dev/null +++ b/cabal-testsuite/PackageTests/LinkerOptions/NonignoredConfigs/cabal.out @@ -0,0 +1,26 @@ +# cabal v2-install +Wrote tarball sdist to /cabal.dist/work/./basic/../dist/sdist/basic-0.1.tar.gz +Resolving dependencies... +Build profile: -w ghc- -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 +# cabal v2-install +Wrote tarball sdist to /cabal.dist/work/./basic/../dist/sdist/basic-0.1.tar.gz +Resolving dependencies... +Build profile: -w ghc- -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 +# cabal v2-install +Wrote tarball sdist to /cabal.dist/work/./basic/../dist/sdist/basic-0.1.tar.gz +Resolving dependencies... +# cabal v2-install +Wrote tarball sdist to /cabal.dist/work/./basic/../dist/sdist/basic-0.1.tar.gz +Resolving dependencies... diff --git a/cabal-testsuite/PackageTests/LinkerOptions/NonignoredConfigs/cabal.test.hs b/cabal-testsuite/PackageTests/LinkerOptions/NonignoredConfigs/cabal.test.hs new file mode 100644 index 00000000000..36041967e5d --- /dev/null +++ b/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 + ]