Look for transitive setup dependency on Cabal when choosing Cabal spec version.

[grayjay]

Please include the following checklist in your PR:

• Patches conform to the coding conventions.
• Any changes that could be relevant to users have been recorded in the changelog.
• The documentation has been updated, if necessary.
• If the change is docs-only, [ci skip] is used to avoid triggering the build bots.

Please also shortly describe how you tested your change. Bonus points for added tests!

---

Fixes #4288.

Previously, new-build only looked for a direct dependency on Cabal when choosing
the spec version to use for running the setup script. When the setup script
only had a transitive dependency on Cabal, cabal used the package's
cabal-version field, which could easily differ from the actual Cabal
dependency's version. Then cabal could pass flags to the setup script that
weren't recognized by the Cabal dependency.

This commit considers any setup dependency on Cabal when choosing the Cabal spec
version.

/cc @ezyang

[hvr]

This is a bit of a coincidence... I'll use this opportunity to ask something... :-)

Yesterday I was looking into how I could inject a qualified per-package constraint on the lower bound matching the declared cabal-version: of a package for its custom-setup component (i.e. (PkgConstraint (ScopeQual (QualSetup "foo") "Cabal") (>= specVersion-of-pkg-foo) )) ... and I was going to ask you about what'd be the best way do this... (I was considering extending the PInfo record with a version-field for recording the specVersion during index conversion, and go from there... not sure if that's the best approach). Any suggestion? :-)

[grayjay]

I'm not completely sure I calculated the closure of the setup dependencies correctly, since I skipped calling elaborateLibSolverId mapDep. I called CD.setupDeps on deps0 directly, because all setup dependencies are library dependencies.

[Ericson2314]

sounds good, but the name through me off haha. We are looking at setup-depends . build-depends* not setup-depends*, right?

[grayjay]

@hvr I think that extending PInfo with the cabal-version makes sense. I'm not sure of the best way to enforce the constraint, though. I think it would need to somehow hook into the validation phase, in https://github.com/haskell/cabal/blob/master/cabal-install/Distribution/Solver/Modular/Validate.hs. In that case, it might be better to add the constraint to the FlaggedDeps in the PInfo (using the Dep type), similar to a setup dependency. Dep is currently defined as:

-- | A dependency (constraint) associates a package name with a constrained
-- instance. It can also represent other types of dependencies, such as
-- dependencies on language extensions.
data Dep qpn = Dep (Maybe UnqualComponentName) qpn CI  -- ^ dependency on a package (possibly for executable)
             | Ext  Extension                          -- ^ dependency on a language extension
             | Lang Language                           -- ^ dependency on a language version
             | Pkg  PkgconfigName VR                   -- ^ dependency on a pkg-config package
deriving Functor

I think that the Cabal constraint would need to be handled slightly differently than the existing Dep data constructor, since it would constrain but not require Cabal (and it would need a different error message when it can't be satisfied). One way to handle the constraint would be to add a Constraint data constructor to Dep that is ignored in the tree building phase (https://github.com/haskell/cabal/blob/master/cabal-install/Distribution/Solver/Modular/Builder.hs) but is treated like a Dep in the validation phase.

@Ericson2314 I'm not sure I understand. What is setup-depends . build-depends*? This PR only affects the choice of the elabSetupScriptCliVersion field in ElaboratedConfiguredPackage for new-build:

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

Lines 282 to 285 in d504388

	-- | The version of the Cabal command line interface that we are using
	-- for this package. This is typically the version of the Cabal lib
	-- that the Setup.hs is built against.
	elabSetupScriptCliVersion :: Version,

[Ericson2314]

That is relation notation. We are looking at the reflexive-transitive closure of build-inputs joined with setup depends, not reflexive-transitive closure of setup depends.

[grayjay]

@Ericson2314 Yes, it isn't following multiple levels of setup-depends.

[grayjay]

Thanks!