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

package-constraints by package file #9462

Closed

Conversation

jasagredo
Copy link
Collaborator

@jasagredo jasagredo commented Nov 21, 2023

Solves #8912 #9477

Manual QA

By adding a field package-constraints in a cabal file and a list of dependencies with version ranges, for each of those dependencies, any dependency declaration that was unbounded will be set to the one on default-constraints:

package-constraints:
  x > 5

library a 
  build-depends: x

library b 
  build-depends: x >5.5

library c
  build-depends: y

Should be understood as:

library a 
  build-depends: x >5

library b 
  build-depends: x >5.5 && >5

library c
  build-depends: y

If the stanza is omitted, then nothing changes wrt previous version.

Notice this should not influence:

  • build-tool-depends
  • pkgconfig-depends
  • transitive dependencies

  • 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.
  • Manual QA notes have been included.
  • Tests have been added. (Ask for help if you don’t know how to write them! Ask for an exemption if tests are too complex for too little coverage!)

@jasagredo jasagredo force-pushed the jasagredo/default-constraints branch 3 times, most recently from 29c217f to 6390c0c Compare November 21, 2023 15:17
@jasagredo jasagredo marked this pull request as ready for review November 21, 2023 15:37
@jasagredo
Copy link
Collaborator Author

jasagredo commented Nov 21, 2023

I requested reviews from some devs, feel free to dismiss otherwise, thanks!

(I don't really know who in particular I should request a review from)

Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

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

This a very visible new feature that will be used by all so, I think, on top of a technical review of the implementation (and the discussion of the design in the original ticket), it would be good to ask the prospective users (RFC somewhere?). I think mentioning this on the fortnightly devs chat may be a good start.

Alternatively, mark it as experimental for one major version, gather feedback from that and change at will (since it's experimental).

@jasagredo jasagredo force-pushed the jasagredo/default-constraints branch 3 times, most recently from 86a3070 to e22b925 Compare November 24, 2023 20:24
@jasagredo jasagredo changed the title default-constraints by package file package-constraints by package file Nov 24, 2023
@jasagredo jasagredo marked this pull request as draft November 24, 2023 20:26
@jasagredo
Copy link
Collaborator Author

Back to draft because it doesn't work in the presence of conditionals

Copy link
Collaborator

@andreabedini andreabedini left a comment

Choose a reason for hiding this comment

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

See the note I have left in the RFC.

In terms of implementation, this should be done in the solver, not in the parser. We should keep the parser faithful to what is written (and we already fail to do so) and the solver already has logic to traverse the GPD and merge the component dependencies toghether (see convCondTree in Distribution.Solver.Modular.IndexConversion).

@gbaz
Copy link
Collaborator

gbaz commented Nov 29, 2023

I don't tend to think this should be done in the solver, because that is complex enough as is, and this need not be done there. I would prefer this were kept in the syntax tree as-is, but applied/simplified before sent to the solver.

(edit: ah -- i see, you mean done not in the midst of solving, but as part of how the solver processes cabal files in building up its tree. that seems a tad more reasonable, though i'd need to think through this more carefully..)

@andreabedini
Copy link
Collaborator

@gbaz yes, I meant in the cabal-install-solver package rather than during the solving process. I could have been more clear. Preprocessing the GPD into a solver specific information is what convCondTree does.

-- | Convert condition trees to flagged dependencies. Mutually
-- recursive with 'convBranch'. See 'convBranch' for an explanation
-- of all arguments preceding the input 'CondTree'.
convCondTree :: Map FlagName Bool -> DependencyReason PN -> PackageDescription -> OS -> Arch -> CompilerInfo -> PN -> FlagInfo ->
Component ->
(a -> BuildInfo) ->
SolveExecutables ->
CondTree ConfVar [Dependency] a -> FlaggedDeps PN
convCondTree flags dr pkg os arch cinfo pn fds comp getInfo solveExes@(SolveExecutables solveExes') (CondNode info ds branches) =
-- Merge all library and build-tool dependencies at every level in
-- the tree of flagged dependencies. Otherwise 'extractCommon'
-- could create duplicate dependencies, and the number of
-- duplicates could grow exponentially from the leaves to the root
-- of the tree.
mergeSimpleDeps $
[ D.Simple singleDep comp
| dep <- ds
, singleDep <- convLibDeps dr dep ] -- unconditional package dependencies
++ L.map (\e -> D.Simple (LDep dr (Ext e)) comp) (allExtensions bi) -- unconditional extension dependencies
++ L.map (\l -> D.Simple (LDep dr (Lang l)) comp) (allLanguages bi) -- unconditional language dependencies
++ L.map (\(PkgconfigDependency pkn vr) -> D.Simple (LDep dr (Pkg pkn vr)) comp) (pkgconfigDepends bi) -- unconditional pkg-config dependencies
++ concatMap (convBranch flags dr pkg os arch cinfo pn fds comp getInfo solveExes) branches
-- build-tools dependencies
-- NB: Only include these dependencies if SolveExecutables
-- is True. It might be false in the legacy solver
-- codepath, in which case there won't be any record of
-- an executable we need.
++ [ D.Simple (convExeDep dr exeDep) comp
| solveExes'
, exeDep <- getAllToolDependencies pkg bi
, not $ isInternal pkg exeDep
]
where
bi = getInfo info

@jasagredo
Copy link
Collaborator Author

Ah I understand your point. I would need some guidance on where to put this and how to carry the constraints all the way there, but yeah, seems reasonable.

@jasagredo jasagredo force-pushed the jasagredo/default-constraints branch from e22b925 to e50d082 Compare December 2, 2023 22:55
@jasagredo jasagredo force-pushed the jasagredo/default-constraints branch from e50d082 to d0338a8 Compare December 3, 2023 01:02
@jasagredo jasagredo marked this pull request as ready for review December 3, 2023 02:27
@@ -795,6 +795,49 @@ describe the package as a whole:
additional hooks, such as the scheme described in the section on
`system-dependent parameters`_.

.. pkg-field:: package-constraints: library list
:since: 3.11
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be 3.12.

.. pkg-field:: package-constraints: library list
:since: 3.11

Starting with **Cabal 3.11**, a new section ``package-constraints``
Copy link
Collaborator

Choose a reason for hiding this comment

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

And the same here.

@@ -19,6 +19,13 @@ relative to the respective preceding *published* version.
versions of the ``Cabal`` library denote unreleased development
branches which have no stability guarantee.

``cabal-version: 3.11``
Copy link
Collaborator

Choose a reason for hiding this comment

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

And here.

@andreabedini
Copy link
Collaborator

@jasagredo I shared my thoughts in the RFC #9477 (comment)

@jasagredo
Copy link
Collaborator Author

Obsoleted by #9570

@jasagredo jasagredo closed this Dec 27, 2023
@jasagredo jasagredo deleted the jasagredo/default-constraints branch December 27, 2023 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants