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

default-package-bounds (RFC #9569) implementation #9570

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

jasagredo
Copy link
Collaborator

@jasagredo jasagredo commented Dec 27, 2023

Solves #8912 #9569

Docs: https://cabal--9570.org.readthedocs.build/en/9570/cabal-package-description-file.html#default-package-bounds

Manual QA

By adding a field default-package-bounds 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-package-bounds:

default-package-bounds
  build-depends:
    text > 5

library a 
  build-depends: text

library b 
  build-depends: text >5.5

library c
  build-depends: y

Should be understood as:

library a 
  build-depends: text >5

library b 
  build-depends: text >5.5

library c
  build-depends: y

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

Notice this should not influence:

  • pkgconfig-depends
  • transitive dependencies
  • setup-depends

  • 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-pakage-bounds branch 2 times, most recently from af6272b to f201573 Compare December 27, 2023 14:55
@michaelpj
Copy link
Collaborator

  • Why not apply to build-tool-depends? That seems surprising.
  • What about subcomponent dependencies? i.e. build-depends: foo:bar. Will default-package-bounds apply there? I think it should, we should probably test this and maybe include it in the examples.

@jasagredo
Copy link
Collaborator Author

jasagredo commented Dec 28, 2023

  1. build-tool-depends uses ExeDependency. But I think it should be doable.

  2. When you say subcomponents you mean sublibraries right? I think you would have to restrict foo for now, but let me check.

EDIT: 2 works out of the box. Constraining either foo of foo:bar in default-package-bounds will constraint the dependency.

@jasagredo
Copy link
Collaborator Author

For 1. it would be unclear what we would be parsing in the default-package-bounds section. Are these Dependency or ExeDependency? I don't think we can make this distinction here, so I'd rather not make it usable for build-tool-depends.

@jasagredo jasagredo force-pushed the jasagredo/default-pakage-bounds branch from f201573 to 4472cac Compare January 8, 2024 20:11
@michaelpj
Copy link
Collaborator

I don't know, it just seems like I would expect this to work:

default-package-bounds: tasty-discover < 3

test-suite a
  build-tool-depends: tasty-discover:tasty-discover

From a user perspective it looks just the same as build-depends: foo:lib

@michaelpj
Copy link
Collaborator

just to reiterate that I continue to be excited about this feature :)

@jasagredo
Copy link
Collaborator Author

Sounds reasonable but how do I disambiguate when parsing? I could create a new AmbiguousDependency which will be either an exe or a lib dependency and apply it wherever it seems it would fit.

@michaelpj
Copy link
Collaborator

I don't actually know enough about cabal to say anything useful 😅

@jasagredo
Copy link
Collaborator Author

jasagredo commented Jan 16, 2024

I have a proposal, what about default-package-bounds using either package names or exe dependencies:

default-package-bounds:
    foo >2
  , bar:exebar >3

library ...
  build-depends:
    foo:sublib  -- will get >2 from `foo >2` in package bounds

The concept of default-...-bounds can conceivably imply that the bound is for the whole package, even if you depend on a sublib only

@jasagredo
Copy link
Collaborator Author

I could parse the following bounds:

data DefaultBounds = DefaultBound    ShortText                       VersionRange
                   | DefaultExeBound PackageName UnqualComponentName VersionRange -- same as ExeDependency

And then apply the DefaultBound to build-depends and pkg-config-depends and the DefaultExeBound to build-tool-depends

@jasagredo jasagredo force-pushed the jasagredo/default-pakage-bounds branch 5 times, most recently from c79b2b7 to 7a8433e Compare January 17, 2024 00:43
@jasagredo
Copy link
Collaborator Author

jasagredo commented Jan 17, 2024

How does this look @michaelpj:

cabal-version:      3.12
name:               foo
version:            0.1.0.0

default-package-bounds:
  , time <0
  , libpkgconf <0
  , bar:bar <0

flag build-depends-conflict
  manual: True
  default: False
  description: cause conflict

flag pkg-config-conflict
  manual: True
  default: False
  description: cause conflict

flag build-tool-conflict
  manual: True
  default: False
  description: cause conflict

library
  default-language: Haskell2010

  if flag(build-depends-conflict)
    build-depends: time
  else
    build-depends: time >0

  if flag(pkg-config-conflict)
    pkgconfig-depends: libpkgconf
  else
    pkgconfig-depends: libpkgconf >0

  if flag(build-tool-conflict)
    build-tool-depends: bar:bar
  else
    build-tool-depends: bar:bar >0

Note that the bounds are used here to make the compilation fail, to easily see that they are applied. And they only come to play if the specific cabal flag is enabled for the conflict. This is the test-case from the test I added.

So now it would apply to all build-depends, pkgconfig-depends and build-tool-depends

@michaelpj
Copy link
Collaborator

That looks nice to me! I didn't know that pkgconfig-depends had bounds! OOI, can you put constraints on pkgconfig packages in constraints?

@jasagredo
Copy link
Collaborator Author

No, constraints hold these:

data PackageConstraint = PackageConstraint ConstraintScope PackageProperty
data PackageProperty
   = PackagePropertyVersion   VersionRange
   | PackagePropertyInstalled
   | PackagePropertySource
   | PackagePropertyFlags     FlagAssignment
   | PackagePropertyStanzas   [OptionalStanza]

So it is only for Haskell package constraints

@jasagredo
Copy link
Collaborator Author

However I just realized that you never asked for a way to provide default bounds for the pkgconfig libraries 😅

Maybe this should only apply to build-tool-depends when qualified and to build-depends when qualified. Yes, I think that would be better

@michaelpj
Copy link
Collaborator

I guess the intuition I want users to have is "this is a bit like what goes in constraints". Or at least, is a subset of that. That's true for the build-depends/build-tool-depends stuff, but would not be true for the pkgconf stuff it seems.

@jasagredo
Copy link
Collaborator Author

Ok I removed the pkgconfig-depends altering via default-package-bounds

@jasagredo jasagredo marked this pull request as draft January 18, 2024 18:52
@jasagredo
Copy link
Collaborator Author

Following up on the discussion on the Cabal meeting.

@gbaz: In this comment you seem to agree to doing this on the solver (or at least say that it seems maybe reasonable) and @andreabedini's concern about being faithful to what is written there sounds reasonable.

Anyways, lets revisit whether the way this is implemented looks right or not. @gbaz raised the point that any cabal-file-consumer that used to parse the dependency bounds from the files no longer would be able to do so.

@gbaz
Copy link
Collaborator

gbaz commented Jan 18, 2024

Hrm. The trick I suppose is that we can't resolve the conditionals prior to the solver, because it will use them in backtracking. But we do need these bounds handled in the solver. So the solution seems to be that at the same step we "unpack" the conditionals into a flat list of possible dependencies, we also resolve this. And that's sensical. However, we have the issue that end-users still won't see the fully resolved deplist with bounds.

What I would suggest is then that just as the solver was modified to handle this sugar at the same time as the conditionals, we also modify our standard conditional resolving things the same way.

In particular, we change the finalizePD and flatten functions to also apply the default-package-bounds?

https://hackage.haskell.org/package/Cabal-syntax-3.10.2.0/docs/Distribution-PackageDescription-Configuration.html#v:finalizePD

@grayjay
Copy link
Collaborator

grayjay commented Jan 19, 2024

Hrm. The trick I suppose is that we can't resolve the conditionals prior to the solver, because it will use them in backtracking. But we do need these bounds handled in the solver. So the solution seems to be that at the same step we "unpack" the conditionals into a flat list of possible dependencies, we also resolve this. And that's sensical. However, we have the issue that end-users still won't see the fully resolved deplist with bounds.

This PR currently applies the default bounds during the conversion to the solver's index format at the start of solving, in convCondTree, not during the resolution of conditionals. The same logic could be applied before the solver. If I understand correctly, default bounds should be applied to all build-depends fields, regardless of whether they are under conditionals.

Another part of the ordering to consider is --allow-newer. Since this feature is like syntactic sugar, I would expect it to be applied earlier.

@jasagredo
Copy link
Collaborator Author

As I said in the meeting, @andreabedini mentioned in ZuriHac that "this will not work with Setup.hs". What did you mean exactly with that Andrea? I have never had to use Setup.hs so I don't know how it differs from normal cabal.

Also @grayjay mentioned (unless I misunderstood) in the meeting that this should probably be moved to Cabal-the-library. If you point me to a specific place I will be happy to explore it and try to move it there.

@mpickering
Copy link
Collaborator

@jasagredo I think @andreabedini raises a good point.

By ./Setup interface he means if you compile a package using..

> cat Setup.hs
import Distribution.Simple; main = defaultMain
> ghc  Setup.hs
> /Setup configure
> ./Setup build

then will the default bounds apply? It sounded like in our conversation yesterday that the default bounds are only applied in the solver which is too late for this to work. Given they are working locally to a package it seems that modifying the bounds in Cabal library (in configure) could be an appropriate place.

Does that make sense?

@jasagredo
Copy link
Collaborator Author

Where can I read how Setup works and place it in my mental model on how cabal works? I have never had to use it ever (cabal has been able to always deal with whatever I needed to compile)

@geekosaur
Copy link
Collaborator

Setup.hs is the CLI interface to the Cabal library. Back before cabal-install and stack existed, the usual way to install a package was

$ runghc Setup.hs configure [parameters]
$ runghc Setup.hs build [parameters]
$ runghc Setup.hs install [parameters]

You still use this interface when installing system-wide or for a system package manager, as opposed to cabal-install or stack or some other build tool that manages its own package store.

Additionally, hadrian (GHC's build system, based on Shake) uses the Cabal API (which `Setup.hs wraps) because it needs direct access to built libraries, and stack uses it because it wants to build into its own package store instead of cabal-install's.

@michaelpj
Copy link
Collaborator

So where's the point where this comes apart from normal build-depends? Do bounds in build-depends get checked by Setup configure but these won't be?

@jasagredo
Copy link
Collaborator Author

These bounds are applied in the solver. However I don't seem to understand why Setup won't use the same path as the solver?

@geekosaur
Copy link
Collaborator

The solver is part of cabal-install, not Cabal. It's inextricably linked to the store and to pkg-config among other things, none of which Cabal and therefore Setup know about.

@mpickering
Copy link
Collaborator

See functions like dependencySatisfiable in Distribution.Simple.Configure.

@jasagredo
Copy link
Collaborator Author

Is there a separate source tree used for Setup or is it intertwined with cabal code? Or does Setup only use Cabal and not cabal, and therefore I should find somewhere in Cabal to put this?

@mpickering
Copy link
Collaborator

./Setup interface only uses code in Cabal library and its dependencies, it is completely distinct from cabal-install.

@andreabedini
Copy link
Collaborator

@jasagredo I need to keep my promise! I'll reach out to you in private.

@jasagredo jasagredo force-pushed the jasagredo/default-pakage-bounds branch from 15970d8 to ab1c14a Compare June 26, 2024 19:18
@jasagredo jasagredo force-pushed the jasagredo/default-pakage-bounds branch from ab1c14a to 8633f93 Compare June 26, 2024 19:40
@jasagredo
Copy link
Collaborator Author

jasagredo commented Jun 26, 2024

I have revamped this PR after talking to @andreabedini (thanks!!) this morning. It is now a section. See the updated documentation. https://cabal--9570.org.readthedocs.build/en/9570/cabal-package-description-file.html#default-package-bounds

It now works also for ./Setup.hs.

doc/cabal-package-description-file.rst Outdated Show resolved Hide resolved
doc/cabal-package-description-file.rst Outdated Show resolved Hide resolved
doc/cabal-package-description-file.rst Outdated Show resolved Hide resolved
@jasagredo jasagredo force-pushed the jasagredo/default-pakage-bounds branch from ef0e28a to d1fb218 Compare June 26, 2024 22:29
@jasagredo jasagredo force-pushed the jasagredo/default-pakage-bounds branch from d1fb218 to 0f17eff Compare June 26, 2024 22:38
Copy link
Collaborator

@grayjay grayjay left a comment

Choose a reason for hiding this comment

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

Also @grayjay mentioned (unless I misunderstood) in the meeting that this should probably be moved to Cabal-the-library. If you point me to a specific place I will be happy to explore it and try to move it there.

I meant that the functionality to apply default bounds should be in Cabal (or Cabal-syntax) so that it can be used by Setup.hs, but it looks like the latest version of the code does that.

I didn't review the whole PR, but I have a few more comments from the code that I read while checking that.

Co-authored-by: Kristen Kozak <[email protected]>
@jasagredo jasagredo force-pushed the jasagredo/default-pakage-bounds branch from d9a7ad6 to 12b23db Compare July 15, 2024 23:02
@jasagredo jasagredo force-pushed the jasagredo/default-pakage-bounds branch from 12b23db to d1d698a Compare July 15, 2024 23:06
@ulysses4ever ulysses4ever added the attention: needs-manual-qa PR is destined for manual QA label Jul 18, 2024
@ulysses4ever ulysses4ever self-requested a review August 21, 2024 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Waiting
Development

Successfully merging this pull request may close these issues.

8 participants