Build plans carry stale flags from previous package versions when package flags change
Closed this issue · 13 comments
Stack init is pulling text into the extra-deps of the build plan, even though the same version is available via the specified snapshot. (Note that lts-5.8 has text-1.2.2.0). This causes project-local recompilation rather than reusing my snapshot's compiled packages for anything that transitively depends on text.
To reproduce
stack unpack strive-2.2.0 && cd strive-2.2.0stack init --resolver lts-5.8 --solver
Expected
Extra-deps:
- gpolyline-0.1.0.1
Actual
extra-deps:
- gpolyline-0.1.0.1
- text-1.2.2.0
Details
stack --version
Version 1.0.4.2 x86_64
(mac)
cabal --version
cabal-install version 1.22.8.0
using version 1.22.7.0 of the Cabal library
That looks like a bug. Let me take look.
Looks like there is something wrong with the lts-5.8 build plan. For the package text it says:
constraints:
library-profiling: true
flags:
integer-simple: false
build-benchmarks: true
skip-build: false
tests: expect-success
haddocks: expect-success
version-range: ! '>=1.2.2.0 && <1.3'
version: '1.2.2.0'
Notice the integer-simple flag being set as false. But in text-1.2.2.0 this flag was removed and integer-pure flag was added. From text.cabal in text-1.2.2.0:
flag developer
description: operate in developer mode
default: False
manual: True
flag integer-pure
description: Use the pure-Haskell integer library instead of GMP
default: False
manual: True
Why is the build plan using the old flag but new package version? The build plan seems to be screwed up.
stack init is adding the text package in extra-deps due to the mismatch in the flags in the build plan and the package flags that the cabal-solver returned. Since flags are not the same we add it in the extra-deps.
lts-3.19 build plan was correct with text-1.2.1.3. It changed to text-1.2.2.0 in lts-3.20 and then onwards all the build plans have wrong/stale integer-simple flag set for text.
I am not familiar with how build plans are generated. @mgsloan ?
Seems like something that needs to be fixed in stackage-curator: fpco/stackage-curator#11
Since I don't think we're going to retroactively update build plans, we may also want to handle this circumstance. AFAICT the non-existent flags aren't causing problems for stack, but in this case it is confusing the cabal-install solver.
There are two flag info items that we consider when we determine whether a snapshot package can be used:
- flags as per the build plans (all automatic flag settings and optionally overrides on manual ones)
- flags returned by cabal solver (automatic flags, manual only if changed from default e.g. via constraints)
We ask cabal solver to use the build plan flags as constraint. Usually if the constraints are honored it returns the same settings as the snapshot. But in case we pass a non-existing flag as a constraint cabal silently ignores it. If a flag setting is not reported in cabal solver output then it indicates two cases:
- The flag is a manual flag with default setting (snapshot may have it at non-default setting)
- The flag does not exist and was silently ignored
If we know that a flag is automatic and is not present in the cabal solver output then we will know that it is a non-existent flag (2nd case above) and raise a warning and ignore it. If it is a manual flag then we will still have the ambiguity between the two cases.
MiniPackageInfo (or build plan) does not tell if a flag is manual so we cannot disambiguate the manual case. But if we assume build plans never or rarely set manual flags then we can ignore the ambiguous manual flag case. Is that a reasonable assumption? or is there any other way to handle this?
@harendra-kumar Interesting, thanks for the details! How about using withCabalLoader and readPackageUnresolvedBS to fetch the packages that are needed to resolve the ambiguity? This seems fine to me, since that stuff will need to get fetched for build anyway.
Loading the cabal files may not be a problem as cabal solver is anyway consulting the packages for solving, we can do the same, just a bit of effort doing it.
Not sure though if just to handle this build plan corruption case its worth writing and maintaining that code. In general we cannot write code on the assumption that build plan is unreliable. If there are no manual flag overrides in the build plans I can easily issue a warning and ignore it.
I quickly coded up a fix to issue a warning (assuming no manual flag overrides in build plan). The output will look like this:
Asking cabal to calculate a build plan...
Trying with packages from lts-5.8 as hard constraints...
WARNING! Ignoring the following spurious flags found in the build plan:
- text: integer-simple = False
Successfully determined a build plan with 1 external dependencies.
@mgsloan can you confirm if there are any cases where a manual flag is overridden in the build plan?
Even when a manual flag is present in the build plan the effect of this fix should be minimal:
- I am not sure if the cabal solver even suggests changes to manual flags. In case it doesn't this problem does not exist.
- In case it does we won't have a problem if it finds a solution using snapshot as hard constraint.
- The only problem will be when we relax all constraints and snapshot has a package with a non-default manual flag whereas cabal suggests to go with default. In that case instead of using an extra dep we will end up using the snapshot package.
I am not sure if the cabal solver even suggests changes to manual flags. In case it doesn't this problem does not exist.
Great point! Sounds like this resolves the theoretical ambiguity :)
I've merged the PR
How is integer-simple spurious in lts-5.11? As far as I understood from comments, a non-existent flag is spurious. text 1.2.2.1 revived integer-simple, and lts-5.11 specifies integer-simple in text 1.2.2.1.
Yet, stack solver says integer-simple is a spurious flag.
This behavior solves my issue, but it makes me question whether it'll introduce other bugs later.
The spurious reporting can be wrong when the build plan explicitly specifies the flag value even though the value specified is the same as default. The assumption of the reporting algo was that the build plan only specifies overrides and not the defaults. The functionality is correct reporting is wrong.
We report spurious when the flag is not present in the cabal solver output but is present in the snapshot. Cabal solver will not explicitly report a flag in its output when it wants the flag to remain at the default value or (of course) if the flag is does not exist. In both cases the decision is the same - to not put the package in extra deps.
We should disable the spurious warning to avoid confusion as it is not possible to unambiguously report spurious.
Removed the spurious warning via commit 40d7ae8 .