Move --allow-{newer,older} back to ./Setup.hs

[maralorn]

Here I am trying to bring back --allow-newer and --allow-older into ./Setup.hs.

This fixes #7445, #5407 and #7859.

I gave this a shot, and it seems to work, but I found the code base hard to navigate.
So please tell me if anything should be organized differently.

I have prepared code for cabal-install to use the methods from Cabal. I did not include in this PR, because I assume cabal-install needs to be backwards compatible with older Cabal versions.

There still seems to be a bug with argument parsing where the following are valid:

• --allow-newer
• --allow-newer=True
• --allow-newer=False
• --allow-newer=*:base
• --allow-newer=*:base,*:aeson
• --allow-newer=*:base,foo:aeson

but not --allow-newer=foo:base.

That is especially weird, because the same parser works in all of those cases in
cabal-install.

I will add changelog entries and documentation later, if this PR is generally well received.

Edit: I have fixed the option parsing. It now behaves exactly like in cabal-install, which is actually simpler than I thought before.

I suggest evaluating this PR commit by commit.

---

• 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.
• Include manual QA notes if your PR relates to cabal-install.

Bonus points for added automated tests!

[ulysses4ever]

Based on the discussion on the issue, I think the change is welcome. We just need to get the reviews.

[jneira]

I will add changelog entries and documentation later, if this PR is generally well received.

hi, thanks for the pr, out of curiosity, any plans to add also tests 🤖
i am afraid qa instructions are focused in cabal-install but maybe this could be an exception if tests are near impossible to write 😝

[maralorn]

Thing is: There are tests for this but they currently reside in cabal-install. I can port them over if desired.

[maralorn]

Okay, so Cabal apparently has no testsuite. I can change the cabal-install tests to use the Types from Cabal but that will break compatibility with other Cabal versions. I'd be thankful for guidance on this.

[ulysses4ever]

I can change the cabal-install tests to use the Types from Cabal but that will break compatibility with other Cabal versions.

Do you mean test components in cabal-install or the cabal-testsuite package? If it's the latter, then that package has no compatibility requirements, I think? If it's the former, I'm not sure if we need to bother about compatibility of the test suite (as long as the exe component builds with ancient Cabal's), but that'd be better double-checked by someone else.

There are some curious discussions about compatibility requirements over at #8998 (esp. #8998 (comment))

[andreabedini]

This only uses relaxPackageDeps to change the package cabal file, and it does not affect the resolving of the dependencies, am I correct?

Also, the syntax --allow-newer=foo:aeson means "remove the upper bounds on the dependency on aeson in all versions of package foo. If we are calling ./Setup.hs on a package that is not foo, this would have no effect at all. Should we issue a warning? or an error?

(I have some doubts in going in this direction (see my comment here) but here I will restrict myself to discussing the implementation here)

[maralorn]

I can change the cabal-install tests to use the Types from Cabal but that will break compatibility with other Cabal versions.

Do you mean test components in cabal-install or the cabal-testsuite package? If it's the latter, then that package has no compatibility requirements, I think? If it's the former, I'm not sure if we need to bother about compatibility of the test suite (as long as the exe component builds with ancient Cabal's), but that'd be better double-checked by someone else.

I didn't know about the testsuite package. We can move tests from cabal-install to the testsuite if that's desired.

[maralorn]

This only uses relaxPackageDeps to change the package cabal file, and it does not affect the resolving of the dependencies, am I correct?

Also, the syntax --allow-newer=foo:aeson means "remove the upper bounds on the dependency on aeson in all versions of package foo. If we are calling ./Setup.hs on a package that is not foo, this would have no effect at all. Should we issue a warning? or an error?

(I have some doubts in going in this direction (see my comment here) but here I will restrict myself to discussing the implementation here)

Well, as written in the other issue Cabal has a very simple solver and that is the thin being affected here.

You are right that the specification of the relaxed dependencies is overly complex for Cabal. Otoh having the same option format for Cabal and cabal-install is certainly easier to users. Also I could imagine a situation where we would pass "pkg-2.3.4:base" so that the override does not apply for older or newer versions. I think that's a nice to have.

We can certainly explain this situation in the docs to limit user confusion.

[maralorn]

Ah, from a look at cabal-install bounds and reading the compat discussion my impression is now, that cabal-install is allowed to have tight constraints on Cabal for being compiled against, it just needs to be able to drive older versions of Cabal, right? Can I have a confirmation on that? In that case I will just modify cabal-install to use the allow newer tooling from Cabal.

[ulysses4ever]

@grayjay could you, please, express your opinion on this issue (moving allow-newer to Cabal and the compatibility story)?

[maralorn]

I am currently refactoring the PR. So I am putting it into draft mode. Please continue with the general discussion.

[andreabedini]

Even if we want to use the same --allow-{newer,older} flags, I don't think we need to use
the machinery in Distribution.Client.Dependecy.

In Distribution.Simple.Configure we have dependencySatisfiable which does the job do checking dependencies. We should adapt that function to relax the version range passed to PackageIndex.lookupInternalDependency in here.

[andreabedini]

Another thought is: if we suppor this in Cabal, can we change cabal-install to rely on this rather than rewriting package descriptions with relaxPackageDeps? This is mentioned in #4203 and required modifying the solver.

[grayjay]

@grayjay could you, please, express your opinion on this issue (moving allow-newer to Cabal and the compatibility story)?

I'm not very familiar with the issue of whether the code should be in Cabal or cabal-install, but I thought that previously the plan was to refactor --allow-newer so that it would be a feature of the dependency solver (issue #4203). If the feature is also needed in the Cabal library, it could also be supported by Cabal's simple dependency solver. Currently --allow-newer/older modifies the PackageDescription, and that has caused issues, such as misleading error messages and code that is difficult to maintain. If we moved the feature to the solver, then we could even have the solver log explain that specific versions are only being considered because of the flag.

I think that we should support --allow-newer/older in Cabal if there is demand for that feature, and we could implement #4203 later. Is there any reason that this PR would make it harder to implement #4203 in the future?

[maralorn]

Thanks for the feedback.

First let me say: Better error output, even by the simple Cabal solver would be awesome. Especially, printing which non-matching versions of a dependency were found would make our lives in Nixpkgs immensely easier.

That being said: Carving out the two days to write this pr was quite hard (and actually a bit irresponsible) for me, and carving out two more to rewrite it is probably not possible. Also, I was very much hoping to backport this PR to our older GHCs in Nixpkgs so the larger the refactoring becomes the harder that gets. This pr is not making the code worse. It is simply using functions which were already there, and is imo a strict improvement. I totally understand that you want to improve the code base, but I would ask you to not force it onto me as an external one-time contributor.

I agree that the solution you describe is nicer, so I will have a look if it looks so easy that I can quickly do it. Will report back.

[grayjay]

@maralorn I should have mentioned that I think #4203 is a much larger project, so it probably doesn't make sense to change this PR to implement it. I just wanted to mention that longer term goal since it relates to this PR.

[maralorn]

In Distribution.Simple.Configure we have dependencySatisfiable which does the job do checking dependencies. We should adapt that function to relax the version range passed to PackageIndex.lookupInternalDependency in here.

I had a look at that. I think I could do it with manageable effort, but I see three problems:

1. I think if this was implemented naively, it would actually make error messages worse. Currently, if I run runhaskell ./Setup.hs configure --allow-newer=Cabal-syntax for Cabal I get:

   Error: Setup.hs: Encountered missing or private dependencies:
   Cabal-syntax >=3.11
   

   Which is arguably better than

   Error: Setup.hs: Encountered missing or private dependencies:
   Cabal-syntax >=3.11 && <3.12
   

   which I would get with the suggested change. (Don’t get confused: In the test environment there is simply no Cabal-syntax available in any version, so the error itself has nothing to do with version bounds.) Maybe even better would be Cabal-syntax >=3.11 && <3.12 (upper bound ignored) but that would be harder to implement.

2. I have not done the deep dive into the code of finalizePD but I have at least the fear that dependencySatisfiable is actually not the only place where version constraints are being looked at. Does anyone know with certainty?

3. (This is a small one:) Porting this way of doing it into cabal-install-solver would be far more involved, so we would have less code reuse between both libraries if we only do this change in Cabal.

[maralorn]

(Just rebased to resolve merge-conflicts, no other changes.)

[andreabedini]

@maralorn Thank for the time you are spending on this, I do appreciate it. I volunteer to push this across the line (if you wish).

@grayjay I don't think this is going to impact #4203 in any way. I think they are orthogonal features, although they appear to have the same name (btw, @ulysses4ever, @fgaz @Kleidukos are we still ok with this? we won't be able to change it later).

[maralorn]

@maralorn Thank for the time you are spending on this, I do appreciate it. I volunteer to push this across the line (if you wish).

That’s very kind, thanks. Let’s first see what needs to be done. I am curious what the answer to my questions above is.

[maralorn]

We discussed this PR today in the cabal-devs meeting.
There was a consensus, that adding the flags is fine and that it makes sense to have them be consistent between cabal-install and Setup.hs. No one voiced any blockers for this PR.

So the only thing missing from my point of view is that we agree on a solution @andreabedini. I had asked some questions above and will be curious to read what you think, when you have time to reply.

[andreabedini]

Hi @maralorn, let me say first that I don't want to be a blocker.

I do think we can achieve your goal with a smaller and simpler change; perhaps even non-breaking.

I worked on this last night. You are correct in that if only change satisfyDeps we end up with a wrong error message. So I changed configureFinalizedPackage instead; remembering which dependencies had their bounds removed and improving the error message exactly like you suggest. (I would have preferred to wrap Dependency, maybe one day)

I think it works, but I would need another couple of day to clean it up, push it somewhere and test it properly.

Re your questions:
2) I am not entirely sure either; it's something worth understanding
3) not sure what you mean by "this way of doing it". At the moment we use relaxPackageDeps to side step the solver, just like you are doing in this PR. Teaching the solver about relaxed constraints is the direction I'd like to take (starting in the naive solver in Cabal) but that is a longer road to take indeed 🙂

If the other maintainers think we should proceed with this PR as it is, I'm happy to shelf my attempt for another day. No hard feelings ☺️

[maralorn]

Hi @maralorn, let me say first that I don't want to be a blocker.

I do think we can achieve your goal with a smaller and simpler change; perhaps even non-breaking.

I am not entirely sure what you mean with non-breaking. Apart from that, most of the change in this PR is moving the datatypes and the parsing for the command line flag into Cabal and I don’t see how or why we would avoid that. If we can keep relaxPackageDeps in cabal-install, okay.

I worked on this last night. You are correct in that if only change satisfyDeps we end up with a wrong error message. So I changed configureFinalizedPackage instead; remembering which dependencies had their bounds removed and improving the error message exactly like you suggest. (I would have preferred to wrap Dependency, maybe one day)

Cool! Better output is a huge win!

I think it works, but I would need another couple of day to clean it up, push it somewhere and test it properly.

Re your questions: 2) I am not entirely sure either; it's something worth understanding 3) not sure what you mean by "this way of doing it". At the moment we use relaxPackageDeps to side step the solver, just like you are doing in this PR. Teaching the solver about relaxed constraints is the direction I'd like to take (starting in the naive solver in Cabal) but that is a longer road to take indeed slightly_smiling_face

Yeah, regarding 2) I think it’s likely that it will just work. I liked editing the package description because it has the abstraction advantage of being very likely to work without understanding or touching the inner workings of the solver.

If the other maintainers think we should proceed with this PR as it is, I'm happy to shelf my attempt for another day. No hard feelings relaxed

If this change could still make it into 3.10 I would love to see that happen, but I assume that ship has sailed. So we have time. I’ll just let you take over. If this gets resolved in, let's say, a month or two, I am totally fine.

[ulysses4ever]

@andreabedini just a gentle ping here. Do you think you could flesh out your experiment in the coming weeks?

[andreabedini]

@andreabedini just a gentle ping here. Do you think you could flesh out your experiment in the coming weeks?

Yes, I will. Apologies and thanks for the ping.

[andreabedini]

I ended up in a rabbit hole trying to separate Cabal flags from cabal-install flags. Even we want to keep the same flags name, I don't think we should use the same representation for AllowNewer since it's really geared toward a more complex use case (the one of cabal-install) but the fact that we just re-use and re-export Cabal configure flags made everything a mess. I think I will try again with the same trick that cabal-install does: Cabal can have an extra set of flags that are not shared with cabal-install. I guess cabal-install could still build on top of Cabal's simpler representation (adding the scope part).

Anyway. The meat of my proposed change is here.

ed2253b#diff-fff9e57c2727835086925d886619deb3fafd33a06e1af760d60d485ae07cec68R1213-R1293

I think it's pretty minimal and a part of it is there to just rework the representation of AllowNewer.

[maralorn]

@andreabedini That looks good to me!

[andreabedini]

I cannot shake the feeling I have thrown a wrench in this PR's wheel. Let me try to first summarise the situation (please correct me if I am mistaken!) and then propose a plan forward:

• @maralorn's desire is to add this feature to Cabal so it can be used right away to improve nixpkgs Haskell's infrastructure. He also seems to be keen this change to be as small as possible since he is planning to backport the patch to older versions of Cabal.
• I am mostly driven by making architecturally sound changes and improve the status of the codebase.
• These two ambitions are not at odds with each other, if it wasn't perhaps for the timeline:
• • @maralorn's PR works right away although making some architectural trade-off: it moves from cabal-install to Cabal code that Cabal is not going to use (--allow-newer=not-the-package-Cabal-is-compiling:some-dependency), and uses package description rewriting which AFAIU is something we desire to move away from (see Don't implement allow-newer/allow-older by modifying PackageDescription #4203).
• • my proposed (still unfinished) implementation works towards Don't implement allow-newer/allow-older by modifying PackageDescription #4203 and refactors the needed types so that they can be split between Cabal and cabal-install, but has run into some difficulty implementing the chosen command line flag [1]. Moreover this unlikely will result in a small patch.
• Maintainers of other build systems might be interested in this too but haven't so far voiced any expectation for this change to happen in the short term.

Note that both changes are API breaking and will require a major version bump (In a comment above I mentioned it could be done in a non-breaking way but I was mistaken: the change in Distribution.Simple.Config is not breaking but one still has to add a flag to ConfigFlags which makes the change API breaking). Accepting this PR and a subsequent refactor implies sequencing two breaking changes with contraddicting vision (moving modules from cabal-install to Cabal and then back to cabal-install).

I am not sure this is acceptable for everyone but I propose the following:

• We extract the smallest change patch that implements this. I am thinking of using my approach in Distribution.Simple.Config and adding a simpler new flag to Cabal which requires no additional types (perhaps sacrificing the special treatment of the caret operator, if that is ok).
• Depending on whether cabal-install needs to be linked to this new patched Cabal, the patch can be extended to simply hide the new flag in cabal-install.
• @maralorn will apply this patch in nixpkgs to all relevant versions of cabal.
• I offer to do the work required and to support the patch (in case there's a issue or something changes in cabal).
• We defer making this change in cabal repo right away; giving us more time to discuss any needed refactors or changes.

I am of course happy to consider suggestions or alternative plan (or straight-on accept a different outcome if there is wide consensus). Let me know what you think, thank you 🙏

[1] --allow-newer becomes an option of both Cabal and cabal-install. I don't think this, di per se, is a problem given their semantic is reasonably close; but Cabal and cabal-install share the same configuration types and we don't have a simple way to add to flags to Cabal which are not cabal-install flags. I could not find a good way to do this, perhaps I am missing a pragmatic solution.

[maralorn]

@andreabedini Thank you for your thoughts. Honestly I am really grateful for you working on this. For years this issue lingered in the "sure, if you want it write it" state and nothing moved forward and now after I invested a bit of time, a cabal contributor is actually commited to do this right. That’s awesome from my point of view.

Also I am really not in a terrible hurry, so don’t feel to stressed about this, anything which makes it into Cabal 3.12 would be totally fine from my point of view. Only reason to urge you on this is that generally live happens and then things fall under the radar. I am for the next few weeks much to busy myself to e.g. apply patches in Nixpkgs.

In any way my plan was to wait until this feature is merged into the master branch before actually doing backports. In theory we can do what we want in Nixpkgs, but we want to be as faithful and respectful of upstream as possible and are careful about applying random patches. Especially I would like to avoid backporting a flag which then suddenly changes in a later version of Cabal or does not get merged at all, because perpetually maintaining a patch like that is a huge burden. On the other hand, I think an exception to this rule can be made, especially if a patch is semi-sanctioned by upstream. So I am fine with your suggested way forward and the help you offered.

Implementation wise I still think having the exact same flag shared between cabal-install and Cabal is the right solution even if we ignore the fact that it’s easier to implement. But I admit, that I might not be objective since that was the solution I implemented. Maybe other Cabal devs can voice their opinions on this.

[Mikolaj]

Please don't stop thinking about this and please kindly post here links to any new related tickets!

[maralorn]

Hey there! I have no clue what the release schedule for Cabal is, but I would be very happy if this could be in the next Cabal release. Can someone tell me the timeline which we would need to meet?

I will gladly fix the current merge conflicts and make all tests run again if there is a reasonable chance of a merge.

[Mikolaj]

@maralorn: yours is a serious change, not a simple bugfix (e.g., a real design discussion is taking place; potentially a RFC might be needed), so I wouldn't expect this can be fast-tracked. We don't have the two positive reviews yet and with this kind of a change we might additionally need an extra buy-in from subsystem maintainers (e.g., @grayjay and @gbaz) and a backward compatibility discussion (I'm making this up, I haven't thought this through yet). The next major release 3.12.1.0 may be out as soon as within a month, so it may be hard to get enough visibility and user testing by then and get this included. All the more reason to resume the discussion to get this finalised by the next major release (probably in around half a year). BTW, it would be unofficially released in a nightly (https://github.com/haskell/cabal/releases/tag/cabal-head) as soon as it's merged so that users can test it in the wild.

[maralorn]

I admit this reply does not make me super happy. I went to a Cabal dev meeting where this PR was discussed and was met with a general consensus. The vibe there was "resolve your discussion with Andrea and then we can go ahead". I was not even informed that there is an RFC process and that I would need to propose anything there. @grayjay already commented on this PR and seemed to be in favor.

I generally don’t want this PR to be fast-tracked. My aim at 3.12 was months ago. But okay, let’s aim for 3.14 then. I just want it to move forward somehow.

@andreabedini would you be willing to work with me on unblocking this? I am available to come to another dev meeting where we could discuss this. I could also just bring this PR in a mergeable state in its current form if you have too much on your plate.

[Mikolaj]

@maralorn: apologies, I didn't mean to discourage you and my conservative estimate of 3.14 is based on a superficial knowledge of this PR and its subject matter. Please prove me wrong and get it ready for 3.12 with some margin for user testing. :)

Re RFC, it's not mandatory in general. It's just a tool. Reviewers and others more informed than myself would know better whether to recommend it and you'd decide whether to go for it.

[andreabedini]

Since @Mikolaj's ping I have devoted few spare moments to my alternative implementation. It's not in a bad shape but I didn't have enough time in these few days to give you an assessment of what's missing.

@maralorn Would you be interested in a couple of pair-coding sessions? I would appreciate it.

[maralorn]

@andreabedini I’d love to. But I am sadly too busy for another few weeks. (I have a big deadline on the 18th of October.)
I am fine with putting a pin in this until then.

[ulysses4ever]

@maralorn @andreabedini any chance you guys can push forward here? 3.12 is still not cut, so there's chance to get the feature there.

[maralorn]

I had a very nice call with @andreabedini and we decided on how to proceed. I currently don‘t have the capacity to work on it, so it will have to wait until I do.

[Mikolaj]

Splendid! Would you like to jot down a couple of sentences, hinting at the core design decisions you arrived at?

[maralorn]

Certainly. So @andreabedini has a branch somewhere where they started their approach (I guess based/inspired by my first attempt.), I will shortly take that branch and build on that.

Differences to this first try:

1. Cabal and cabal-install will have different options for --allow-newer because Cabal only ever builds one package it does not make sense to specify for which package to loosen the bounds.
2. We implement the loosening by properly ignoring the bounds in the (Cabal-)planner instead of removing the bounds higher up the call stack.
3. If possible we will try to modify the error message to a) show which bounds were ignored and b) while we are at it print which packages where found which did not match the version bounds.

[ulysses4ever]

@andreabedini @maralorn are there any updates on this work? It's sad to see a good PR lingering for so long...

[maralorn]

I agree. I tried to work on this at zurihac but I had a lot of cache misses on the details because this took so long …

I still want to work on this, but live happens.