Fix repl discarding --build-depends after allow-newer/older #8732

wismill · 2023-02-03T14:23:55Z

Fix REPL discarding --build-depends when using allow-newer or allow-older.

Fixes #6859
Fixes #7081
Related issues: #8504, #5900.

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.

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

wismill · 2023-02-03T14:33:02Z

Note: this is currently hacky. It fixes my use case (namely run doctest), but I am not sure it fixes it in general.

The following points need review:

The combination of dependencies lists in mkCondNode: currently done naively.
The hack on condTreeConstraints field: we have a function g :: BuildInfo → f BuildInfo but we need to update [Dependency].

Cabal-syntax/src/Distribution/Types/GenericPackageDescription.hs

wismill · 2023-02-03T20:08:05Z

New version, which hopefully is correct and much simplier.

ulysses4ever · 2023-02-05T14:57:48Z

I hope to attend to this PR ASAP. I really hope we can squeeze it into 3.10 @Mikolaj

ulysses4ever

This looks great. Two requests (apart from minor inline comments):

A changelog file
A better commit sequence. Please, force-push your branch so that it doesn't have the previous attempt, and has the cleanup work in a separate commit: this will make reviewers' live resort

cabal-testsuite/PackageTests/ReplBuildDepends/cabal.test.hs

Cabal-syntax/src/Distribution/Types/GenericPackageDescription.hs

cabal-install/src/Distribution/Client/CmdRepl.hs

Mikolaj · 2023-02-06T08:44:15Z

I hope to attend to this PR ASAP. I really hope we can squeeze it into 3.10 @Mikolaj

Yes, we are only in semi-freeze, so if there are good reasons, we should merge it into 3.10. E.g., improving CI is a good reason, because 3.10 is going to stay there longer, since we need some other changes before release.

wismill · 2023-02-06T12:34:25Z

Fixed based on feedback, rebased and squashed.

ulysses4ever · 2023-02-06T16:07:42Z

@wismill amazing, thanks! One more thing: could you, please, explain in the commit message (1) what this change does, in plain English, as much as possible, and (2) why you think this fixes the issue?

cabal-install/src/Distribution/Client/CmdRepl.hs

wismill · 2023-02-06T19:53:16Z

@ulysses4ever I added proper comment & commit message. Unfortunately there is an issue withe the recent release of happy.

Mikolaj

Splendid. I'm now doubly edified: from the code comment and commit comment.

Mikolaj · 2023-02-06T19:54:54Z

Yes, our colleague has already reported the happy problem and we count on them fixing it ASAP so that we don't have to exclude the happy version ourselves. Happy times.

wismill · 2023-02-06T20:03:41Z

Note that I am new to Cabal and although I think the patch is consistent, please double check it!

There might be more places where the pattern L.allCondTrees . traverseCondTreeC might be an issue, e.g. transformAllBuildDepends.

ulysses4ever

Typo in the commit message: "manipuling". Other than that — terrific, thanks!

Mikolaj · 2023-02-06T21:53:22Z

There might be more places where the pattern L.allCondTrees . traverseCondTreeC might be an issue, e.g. transformAllBuildDepends.

Please kindly open a ticket about that. We don't have to do everything in one go.

Mikolaj · 2023-02-06T23:04:37Z

Note that I am new to Cabal and although I think the patch is consistent, please double check it!

Given we want to backport this into 3.10, let's ask our resident gurus @grayjay, @gbaz and @fgaz for a quick look.

cabal-testsuite/PackageTests/ReplBuildDepends/cabal.allow-newer.out

Mikolaj · 2023-02-09T19:11:06Z

@wismill: feel free to set a label (squash+merge_me I guess), which will still give @fgaz 2 days to ponder and grace us with a comment, if @fgaz so chooses.

BTW, let's not forget to open a ticket for the other occurrences of the same pattern in the codebase. Even if we later close the ticket with "nothing to do".

Mikolaj · 2023-02-09T21:12:11Z

@mergify rebase

Previously the function `\f -> L.allCondTrees $ traverseCondTreeC f` was used to add the dependencies, but manipulating `CondTree` this way does not update the nested fields `targetBuildDepends` of the tree, only the conditions. It worked merely by chance if one does not further process these fields: this explains why options like `--allow-newer` or `allow-older` were incompatible with `--build-depends`. Using `L.traverseBuildInfos . L.targetBuildDepends` ensures `targetBuildDepends` fields and conditions are all updated consistently.

mergify · 2023-02-09T21:12:51Z

⚠️ This pull request got rebased on behalf of a random user of the organization.
This behavior will change on the 1st February 2023, Mergify will pick the author of the pull request instead.

To get the future behavior now, you can configure bot_account options (e.g.: bot_account: { author } or update_bot_account: { author }.

Or you can create a dedicated github account for squash and rebase operations, and use it in different bot_account options.

mergify · 2023-02-09T21:12:51Z

rebase

✅ Branch has been successfully rebased

ulysses4ever · 2023-02-13T12:27:29Z

Regarding backport to 3.10: I'm of two minds here. On one hand, I truly believe this can make 3.10 release greater and brighter. On the other, I think it may break some stuff, perhaps — at least, I didn't see anyone on this thread pleading that it can't. So, perhaps, it's Mikolaj's call.

Mikolaj · 2023-02-13T16:35:30Z

@ulysses4ever: I asked people at the last devs meeting to give this an extra scrutiny specifically due to the late semi-freeze backporting and we did get a lot of reviews in. However, I find your sudden lack of faith disturbing. :)

Mikolaj · 2023-02-13T17:42:47Z

@wismill: do we have the ticket about "the other occurrences of the same wrong pattern in our codebase"? If so, it would be useful to link it here for cross-referencing.

Mikolaj · 2023-02-13T19:13:01Z

@mergify backport 3.10

* Fix repl discarding `build-depends` argument Previously the function `\f -> L.allCondTrees $ traverseCondTreeC f` was used to add the dependencies, but manipulating `CondTree` this way does not update the nested fields `targetBuildDepends` of the tree, only the conditions. It worked merely by chance if one does not further process these fields: this explains why options like `--allow-newer` or `allow-older` were incompatible with `--build-depends`. Using `L.traverseBuildInfos . L.targetBuildDepends` ensures `targetBuildDepends` fields and conditions are all updated consistently. * Add test * Add changelog * Cleanup --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit e9b4744)

mergify · 2023-02-13T19:13:17Z

backport 3.10

✅ Backports have been created

#8762 Fix repl discarding --build-depends (backport #8732) has been created for branch 3.10

Fix repl discarding --build-depends (backport #8732)

wismill force-pushed the wip/fix6859 branch from bfbf2fb to 48a3659 Compare February 3, 2023 14:25

wismill commented Feb 3, 2023

View reviewed changes

Cabal-syntax/src/Distribution/Types/GenericPackageDescription.hs Outdated Show resolved Hide resolved

wismill changed the title ~~Fix HasBuildInfos instance of GenericPackageDescription~~ Fix repl discarding --build-depends Feb 4, 2023

wismill force-pushed the wip/fix6859 branch from b162f9f to 1907598 Compare February 4, 2023 06:09

wismill requested a review from ulysses4ever February 4, 2023 06:31

wismill marked this pull request as ready for review February 4, 2023 06:31

wismill added the cabal-install: cmd/repl label Feb 4, 2023

wismill requested a review from grayjay February 5, 2023 08:03

wismill mentioned this pull request Feb 5, 2023

cabal-install >= 3.4 regression: cabal repl -b fails to install packages in combination with --allow-newer #7081

Closed

ulysses4ever added the attention: needs-review label Feb 5, 2023

ulysses4ever requested changes Feb 5, 2023

View reviewed changes

cabal-testsuite/PackageTests/ReplBuildDepends/cabal.test.hs Show resolved Hide resolved

Cabal-syntax/src/Distribution/Types/GenericPackageDescription.hs Show resolved Hide resolved

cabal-install/src/Distribution/Client/CmdRepl.hs Outdated Show resolved Hide resolved

wismill force-pushed the wip/fix6859 branch from 5f3c8b6 to 00558af Compare February 6, 2023 11:03

Mikolaj reviewed Feb 6, 2023

View reviewed changes

cabal-install/src/Distribution/Client/CmdRepl.hs Show resolved Hide resolved

wismill force-pushed the wip/fix6859 branch from 00558af to e995366 Compare February 6, 2023 19:45

Mikolaj approved these changes Feb 6, 2023

View reviewed changes

ulysses4ever approved these changes Feb 6, 2023

View reviewed changes

wismill force-pushed the wip/fix6859 branch from e995366 to 616f919 Compare February 6, 2023 22:20

gbaz approved these changes Feb 9, 2023

View reviewed changes

grayjay reviewed Feb 9, 2023

View reviewed changes

cabal-testsuite/PackageTests/ReplBuildDepends/cabal.allow-newer.out Show resolved Hide resolved

Mikolaj requested a review from fgaz February 9, 2023 18:27

wismill force-pushed the wip/fix6859 branch 2 times, most recently from 3c4f715 to b4a4f92 Compare February 9, 2023 18:40

wismill added the squash+merge me Tell Mergify Bot to squash-merge label Feb 9, 2023

wismill added 4 commits February 9, 2023 21:12

Add test

9fdbb54

Add changelog

bc4ba70

Cleanup

8a51101

cocreature force-pushed the wip/fix6859 branch from b4a4f92 to 8a51101 Compare February 9, 2023 21:12

mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Feb 11, 2023

Merge branch 'master' into wip/fix6859

4aafbf8

mergify bot merged commit e9b4744 into haskell:master Feb 11, 2023

haskell deleted a comment from mergify bot Feb 13, 2023

mergify bot mentioned this pull request Feb 13, 2023

Fix repl discarding --build-depends (backport #8732) #8762

Merged

mergify bot added a commit that referenced this pull request Feb 13, 2023

Merge pull request #8762 from haskell/mergify/bp/3.10/pr-8732

05b0630

Fix repl discarding --build-depends (backport #8732)

ulysses4ever changed the title ~~Fix repl discarding --build-depends~~ Fix repl discarding --build-depends after allow-newer/older Apr 18, 2023

ulysses4ever mentioned this pull request Apr 18, 2023

cabal repl --allow-newer #1835

Closed

philderbeast mentioned this pull request Jan 18, 2024

Use package groups #9565

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix repl discarding --build-depends after allow-newer/older #8732

Fix repl discarding --build-depends after allow-newer/older #8732

wismill commented Feb 3, 2023 •

edited by fgaz

Loading

wismill commented Feb 3, 2023

wismill commented Feb 3, 2023

ulysses4ever commented Feb 5, 2023 •

edited

Loading

ulysses4ever left a comment •

edited

Loading

Mikolaj commented Feb 6, 2023

wismill commented Feb 6, 2023

ulysses4ever commented Feb 6, 2023

wismill commented Feb 6, 2023

Mikolaj left a comment

Mikolaj commented Feb 6, 2023

wismill commented Feb 6, 2023

ulysses4ever left a comment

Mikolaj commented Feb 6, 2023

Mikolaj commented Feb 6, 2023

Mikolaj commented Feb 9, 2023 •

edited

Loading

Mikolaj commented Feb 9, 2023

mergify bot commented Feb 9, 2023

mergify bot commented Feb 9, 2023

ulysses4ever commented Feb 13, 2023 •

edited

Loading

Mikolaj commented Feb 13, 2023

Mikolaj commented Feb 13, 2023

Mikolaj commented Feb 13, 2023

mergify bot commented Feb 13, 2023

Fix repl discarding --build-depends after allow-newer/older #8732

Fix repl discarding --build-depends after allow-newer/older #8732

Conversation

wismill commented Feb 3, 2023 • edited by fgaz Loading

wismill commented Feb 3, 2023

wismill commented Feb 3, 2023

ulysses4ever commented Feb 5, 2023 • edited Loading

ulysses4ever left a comment • edited Loading

Choose a reason for hiding this comment

Mikolaj commented Feb 6, 2023

wismill commented Feb 6, 2023

ulysses4ever commented Feb 6, 2023

wismill commented Feb 6, 2023

Mikolaj left a comment

Choose a reason for hiding this comment

Mikolaj commented Feb 6, 2023

wismill commented Feb 6, 2023

ulysses4ever left a comment

Choose a reason for hiding this comment

Mikolaj commented Feb 6, 2023

Mikolaj commented Feb 6, 2023

Mikolaj commented Feb 9, 2023 • edited Loading

Mikolaj commented Feb 9, 2023

mergify bot commented Feb 9, 2023

mergify bot commented Feb 9, 2023

✅ Branch has been successfully rebased

ulysses4ever commented Feb 13, 2023 • edited Loading

Mikolaj commented Feb 13, 2023

Mikolaj commented Feb 13, 2023

Mikolaj commented Feb 13, 2023

mergify bot commented Feb 13, 2023

✅ Backports have been created

wismill commented Feb 3, 2023 •

edited by fgaz

Loading

ulysses4ever commented Feb 5, 2023 •

edited

Loading

ulysses4ever left a comment •

edited

Loading

Mikolaj commented Feb 9, 2023 •

edited

Loading

ulysses4ever commented Feb 13, 2023 •

edited

Loading