Skip to content

Commit

Permalink
Plan build when test components are added #1399
Browse files Browse the repository at this point in the history
+ changes the ensureConfig logic to ignore the list of components, as it
does not affect the config of the package.

+ avoid reconfigures when the new deps are a subset of the old deps
  • Loading branch information
mgsloan committed Nov 24, 2015
1 parent 0c5b396 commit ac57440
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 8 deletions.
9 changes: 2 additions & 7 deletions src/Stack/Build/ConstructPlan.hs
Original file line number Diff line number Diff line change
Expand Up @@ -537,8 +537,8 @@ checkDirtiness ps installed package present wanted = do

describeConfigDiff :: Config -> ConfigCache -> ConfigCache -> Maybe Text
describeConfigDiff config old new
| configCacheDeps old /= configCacheDeps new = Just "dependencies changed"
| not $ Set.null $ Set.filter isLibExe newComponents =
| not (configCacheDeps new `Set.isSubsetOf` configCacheDeps old) = Just "dependencies changed"
| not $ Set.null newComponents =
Just $ "components added: " `T.append` T.intercalate ", "
(map (decodeUtf8With lenientDecode) (Set.toList newComponents))
| not (configCacheHaddock old) && configCacheHaddock new = Just "rebuilding with haddocks"
Expand All @@ -550,11 +550,6 @@ describeConfigDiff config old new
]
| otherwise = Nothing
where
isLibExe t = not $ any (`S8.isPrefixOf` t)
[ "test:"
, "bench:"
]

-- options set by stack
isStackOpt t = any (`T.isPrefixOf` t)
[ "--dependency="
Expand Down
8 changes: 7 additions & 1 deletion src/Stack/Build/Execute.hs
Original file line number Diff line number Diff line change
Expand Up @@ -627,13 +627,19 @@ ensureConfig newConfigCache pkgDir ExecuteEnv {..} announce cabal cabalfp = do
if boptsReconfigure eeBuildOpts
then return True
else do
-- We can ignore the components portion of the config
-- cache, because it's just used to inform 'construct
-- plan that we need to plan to build additional
-- components. These components don't affect the actual
-- package configuration.
let ignoreComponents cc = cc { configCacheComponents = Set.empty }
-- Determine the old and new configuration in the local directory, to
-- determine if we need to reconfigure.
mOldConfigCache <- tryGetConfigCache pkgDir

mOldCabalMod <- tryGetCabalMod pkgDir

return $ mOldConfigCache /= Just newConfigCache
return $ fmap ignoreComponents mOldConfigCache /= Just (ignoreComponents newConfigCache)
|| mOldCabalMod /= Just newCabalMod
let ConfigureOpts dirs nodirs = configCacheOpts newConfigCache
when needConfig $ withMVar eeConfigureLock $ \_ -> do
Expand Down

2 comments on commit ac57440

@mgsloan
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@snoyberg @borsboom I think this is all sensible, but I want to makes sure. In order to fix #1399 , we consider any added component to cause config dirtiness while constructing the plan. The restriction to just lib / exes came from before #1166 (all-in-one-builds) was implemented, and should have been removed.

However, changing this caused rebuilds when switching between build and test. This required two resolutions:

  • Ignoring configCacheComponents when determining if we need to reconfigure the package. This is fine since it doesn't actually affect the arguments passed to the configuration. EDIT: actually, it makes even more sense to only configure when configCacheOpts changes - I've made that change: 7cd87cb
  • Allowing configCacheDeps to be a subset of the old set of deps. I think this happens because it's comparing the present deps against all the deps. I can't think of any cases where allowing a subset of the old deps changes things. In all the cases that matter (e.g. removing a dep from a cabal file), file dirtiness will cause it to be rebuilt.

Also, this seems like an important thing to fix. Might be good to release a 0.1.8.1 soonish, once we're sure this doesn't introduce any bad regressions.

@snoyberg
Copy link
Contributor

Choose a reason for hiding this comment

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

Without diving deeply into this, I've reviewed, and I think the change is fine.

Please sign in to comment.