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

Simple refactoring #5926

Merged
merged 7 commits into from
Nov 12, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 6 additions & 4 deletions src/Stack/Build/ConstructPlan.hs
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ import System.Environment (lookupEnv)
import System.IO (putStrLn)
import RIO.PrettyPrint
import RIO.Process (findExecutable, HasProcessContext (..))
import Stack.Types.Dependency (DepValue(DepValue), DepType (AsLibrary))

data PackageInfo
=
Expand Down Expand Up @@ -684,7 +685,8 @@ addEllipsis t
addPackageDeps :: Package -> M (Either ConstructPlanException (Set PackageIdentifier, Map PackageIdentifier GhcPkgId, IsMutable))
addPackageDeps package = do
ctx <- ask
deps' <- packageDepsWithTools package
checkAndWarnForUnknownTools package
let deps' = packageDeps package
deps <- forM (Map.toList deps') $ \(depname, DepValue range depType) -> do
eres <- addDep depname
let getLatestApplicableVersionAndRev :: M (Maybe (Version, BlobKey))
Expand Down Expand Up @@ -925,8 +927,8 @@ psLocation PSRemote{} = Snap

-- | Get all of the dependencies for a given package, including build
-- tool dependencies.
packageDepsWithTools :: Package -> M (Map PackageName DepValue)
packageDepsWithTools p = do
checkAndWarnForUnknownTools :: Package -> M ()
checkAndWarnForUnknownTools p = do
-- Check whether the tool is on the PATH before warning about it.
warnings <- fmap catMaybes $ forM (Set.toList $ packageUnknownTools p) $
\name@(ExeName toolName) -> do
Expand All @@ -938,7 +940,7 @@ packageDepsWithTools p = do
Left _ -> pure $ Just $ ToolWarning name (packageName p)
Right _ -> pure Nothing
tell mempty { wWarnings = (map toolWarningText warnings ++) }
pure $ packageDeps p
pure ()

-- | Warn about tools in the snapshot definition. States the tool name
-- expected and the package name using it.
Expand Down
8 changes: 4 additions & 4 deletions src/Stack/Build/Haddock.hs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
{-# LANGUAGE FlexibleContexts #-}
{-# LANGUAGE MultiParamTypeClasses #-}
{-# LANGUAGE OverloadedStrings #-}
{-# LANGUAGE RecordWildCards #-}
{-# LANGUAGE NamedFieldPuns #-}
Copy link
Member

Choose a reason for hiding this comment

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

I did not follow what prompted this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change was motivated by a simplified tracking of what is used from the Package type (as I intend to change this type to have a more cabal-like structure as stated in the issue, and thus I have to know what is used where). It's not strictly necessary, but I believe RecordWildCards is a bad and unnecessary extension (especially in this case, when 10+ fields are used it's questionable but obviously unseful).

{-# LANGUAGE ScopedTypeVariables #-}

-- | Generate haddocks
Expand Down Expand Up @@ -107,7 +107,7 @@ generateLocalHaddockIndex
generateLocalHaddockIndex bco localDumpPkgs locals = do
let dumpPackages =
mapMaybe
(\LocalPackage{lpPackage = Package{..}} ->
(\LocalPackage{lpPackage = Package{packageName, packageVersion}} ->
F.find
(\dp -> dpPackageIdent dp == PackageIdentifier packageName packageVersion)
localDumpPkgs)
Expand Down Expand Up @@ -139,7 +139,7 @@ generateDepsHaddockIndex bco globalDumpPkgs snapshotDumpPkgs localDumpPkgs local
depDocDir
where
getGhcPkgId :: LocalPackage -> Maybe GhcPkgId
getGhcPkgId LocalPackage{lpPackage = Package{..}} =
getGhcPkgId LocalPackage{lpPackage = Package{packageName, packageVersion}} =
let pkgId = PackageIdentifier packageName packageVersion
mdpPkg = F.find (\dp -> dpPackageIdent dp == pkgId) localDumpPkgs
in fmap dpGhcPkgId mdpPkg
Expand Down Expand Up @@ -219,7 +219,7 @@ generateHaddockIndex descr bco dumpPackages docRelFP destDir = do
fromString (toFilePath destIndexFile)
where
toInterfaceOpt :: DumpPackage -> IO (Maybe ([String], UTCTime, Path Abs File, Path Abs File))
toInterfaceOpt DumpPackage {..} =
toInterfaceOpt DumpPackage {dpHaddockInterfaces, dpPackageIdent, dpHaddockHtml} =
case dpHaddockInterfaces of
[] -> pure Nothing
srcInterfaceFP:_ -> do
Expand Down
18 changes: 1 addition & 17 deletions src/Stack/Build/Source.hs
Original file line number Diff line number Diff line change
Expand Up @@ -315,25 +315,13 @@ loadLocalPackage pp = do
{ packageConfigEnableTests = not $ Set.null tests
, packageConfigEnableBenchmarks = not $ Set.null benches
}
testconfig = config
{ packageConfigEnableTests = True
, packageConfigEnableBenchmarks = False
}
benchconfig = config
{ packageConfigEnableTests = False
, packageConfigEnableBenchmarks = True
}
Comment on lines -318 to -325
Copy link
Member

Choose a reason for hiding this comment

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

For my benefit, could you explain further why these can be safely deleted? I am not across the architectural considerations and would appreciate some help in understanding them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly I don't know why it existed in the first place, as it's never been used as far as I know.

Copy link
Contributor Author

@theobat theobat Nov 12, 2022

Choose a reason for hiding this comment

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

The fields have been there for a very long time (7+ years) and I don't know at which point they stopped being used.
The reason why it's important to remove them is that, we should have dependencies tracked at the component level for mixins/backpack support, not at the LocalPackage/Package level.


-- We resolve the package in 4 different configurations:
-- We resolve the package in 2 different configurations:
--
-- - pkg doesn't have tests or benchmarks enabled.
--
-- - btpkg has them enabled if they are present.
--
-- - testpkg has tests enabled, but not benchmarks.
--
-- - benchpkg has benchmarks enabled, but not tests.
--
-- The latter two configurations are used to compute the deps
-- when --enable-benchmarks or --enable-tests are configured.
-- This allows us to do an optimization where these are passed
Expand All @@ -343,8 +331,6 @@ loadLocalPackage pp = do
btpkg
| Set.null tests && Set.null benches = Nothing
| otherwise = Just (resolvePackage btconfig gpkg)
testpkg = resolvePackage testconfig gpkg
benchpkg = resolvePackage benchconfig gpkg

componentFiles <- memoizeRefWith $ fst <$> getPackageFilesForTargets pkg (ppCabalFP pp) nonLibComponents

Expand Down Expand Up @@ -372,8 +358,6 @@ loadLocalPackage pp = do

pure LocalPackage
{ lpPackage = pkg
, lpTestDeps = dvVersionRange <$> packageDeps testpkg
, lpBenchDeps = dvVersionRange <$> packageDeps benchpkg
, lpTestBench = btpkg
, lpComponentFiles = componentFiles
, lpBuildHaddocks = cpHaddocks (ppCommon pp)
Expand Down
3 changes: 2 additions & 1 deletion src/Stack/Package.hs
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ import System.IO.Error
import RIO.Process
import RIO.PrettyPrint
import qualified RIO.PrettyPrint as PP (Style (Module))
import Stack.Types.Dependency (DepValue(..), DepType (..))

data Ctx = Ctx { ctxFile :: !(Path Abs File)
, ctxDistDir :: !(Path Abs Dir)
Expand Down Expand Up @@ -143,7 +144,7 @@ packageFromPackageDescription packageConfig pkgFlags (PackageDescriptionPair pkg
, packageFlags = packageConfigFlags packageConfig
, packageDefaultFlags = M.fromList
[(flagName flag, flagDefault flag) | flag <- pkgFlags]
, packageAllDeps = S.fromList (M.keys deps)
, packageAllDeps = M.keysSet deps
, packageLibraries =
let mlib = do
lib <- library pkg
Expand Down
2 changes: 0 additions & 2 deletions src/Stack/SDist.hs
Original file line number Diff line number Diff line change
Expand Up @@ -322,8 +322,6 @@ readLocalPackage pkgDir = do
, lpCabalFile = cabalfp
-- NOTE: these aren't the 'correct values, but aren't used in
-- the usage of this function in this module.
, lpTestDeps = Map.empty
, lpBenchDeps = Map.empty
, lpTestBench = Nothing
, lpBuildHaddocks = False
, lpForceDirty = False
Expand Down
33 changes: 33 additions & 0 deletions src/Stack/Types/Dependency.hs
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
{-# LANGUAGE NoImplicitPrelude #-}

module Stack.Types.Dependency
(DepValue(..)
,DepType(..)
)
where

import Stack.Prelude
import Distribution.Types.VersionRange (VersionRange)
import Stack.Types.Version (intersectVersionRanges)


-- | The value for a map from dependency name. This contains both the
-- version range and the type of dependency, and provides a semigroup
-- instance.
data DepValue = DepValue
{ dvVersionRange :: !VersionRange
, dvType :: !DepType
}
deriving (Show, Typeable)
instance Semigroup DepValue where
DepValue a x <> DepValue b y = DepValue (intersectVersionRanges a b) (x <> y)

-- | Is this package being used as a library, or just as a build tool?
-- If the former, we need to ensure that a library actually
-- exists. See
-- <https://github.com/commercialhaskell/stack/issues/2195>
data DepType = AsLibrary | AsBuildTool
deriving (Show, Eq)
instance Semigroup DepType where
AsLibrary <> _ = AsLibrary
AsBuildTool <> x = x
32 changes: 1 addition & 31 deletions src/Stack/Types/Package.hs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import Stack.Types.GhcPkgId
import Stack.Types.NamedComponent
import Stack.Types.SourceMap
import Stack.Types.Version
import Stack.Types.Dependency (DepValue)

-- | Type representing exceptions thrown by functions exported by the
-- "Stack.Package" module.
Expand Down Expand Up @@ -139,27 +140,6 @@ data Package =
packageIdent :: Package -> PackageIdentifier
packageIdent p = PackageIdentifier (packageName p) (packageVersion p)

-- | The value for a map from dependency name. This contains both the
-- version range and the type of dependency, and provides a semigroup
-- instance.
data DepValue = DepValue
{ dvVersionRange :: !VersionRange
, dvType :: !DepType
}
deriving (Show,Typeable)
instance Semigroup DepValue where
DepValue a x <> DepValue b y = DepValue (intersectVersionRanges a b) (x <> y)

-- | Is this package being used as a library, or just as a build tool?
-- If the former, we need to ensure that a library actually
-- exists. See
-- <https://github.com/commercialhaskell/stack/issues/2195>
data DepType = AsLibrary | AsBuildTool
deriving (Show, Eq)
instance Semigroup DepType where
AsLibrary <> _ = AsLibrary
AsBuildTool <> x = x

packageIdentifier :: Package -> PackageIdentifier
packageIdentifier pkg =
PackageIdentifier (packageName pkg) (packageVersion pkg)
Expand Down Expand Up @@ -197,11 +177,6 @@ data BuildInfoOpts = BuildInfoOpts
, bioCabalMacros :: Path Abs File
} deriving Show

-- | Files to get for a cabal package.
data CabalFileType
= AllFiles
| Modules

-- | Files that the package depends on, relative to package directory.
-- Argument is the location of the Cabal file
newtype GetPackageFiles = GetPackageFiles
Expand Down Expand Up @@ -283,11 +258,6 @@ data LocalPackage = LocalPackage
-- "buildable: false".
, lpWanted :: !Bool -- FIXME Should completely drop this "wanted" terminology, it's unclear
-- ^ Whether this package is wanted as a target.
, lpTestDeps :: !(Map PackageName VersionRange)
-- ^ Used for determining if we can use --enable-tests in a normal build.
, lpBenchDeps :: !(Map PackageName VersionRange)
-- ^ Used for determining if we can use --enable-benchmarks in a normal
-- build.
, lpTestBench :: !(Maybe Package)
-- ^ This stores the 'Package' with tests and benchmarks enabled, if
-- either is asked for by the user.
Expand Down
1 change: 1 addition & 0 deletions stack.cabal
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,7 @@ library
other-modules:
Path.Extended
Stack.Types.Cache
Stack.Types.Dependency
autogen-modules:
Paths_stack
hs-source-dirs:
Expand Down