Fix --repl-options parsing

[bacchanalia]

Bug #7867

[jneira]

Hi, thanks for the standalone fix, it would need a regression test, but if you have tested manually and it is included in your main pr in some way it would be fine for me.
A changelog entry would be great.

[jneira]

As noted by @bacchanalia the bug is not in any release so the changelog would not be needed and the main pr is already testing it, could you add a brief note about what test is and how is testing the bug?

[bacchanalia]

In the latest design of #7851, I use replOptionsFlags to pass the script location between cabal-install and Cabal, so either cabal-testsuite/PackageTests/NewBuild/CmdRepl/Script or cabal-testsuite/PackageTests/NewBuild/CmdRepl/ScriptRerun would catch it because the repl would fail to load from the script file being passed multiple times.

[jneira]

The pr (#7578) by @sirlensalot had specific tests so i hope this change will not break the functionality added there. I would ask the pr author to take a look and confirm it, thanks!

[jneira]

looks good to me given the notes about the changelog and tests, thanks for the fix!

[Mikolaj]

I think the backport of this bug is in 3.6.2 (#7593), ain't it so?

[jneira]

it seems #7593 was closed wisely by @fgaz without merging it

[Mikolaj]

Wow, I missed it. Well done, team. :)

[Mikolaj]

Super.

[jneira]

no need for backport then (i was not sure if the bug was backported after the release so thanks for link the closed backport)

i would wait some time for the review of the original pr author before merging

[sergv]

Although I'm not the original author, I can confirm that the commit is correct. I did encounter this problem myself and did a bit of investigation.

The original commit (cf 785ea62#diff-793c63febac9c808425678cd5535233aebb1aca838a829a665f85d4452f62346L1766) did the following change

-replOptions :: ShowOrParseArgs -> [OptionField [String]]
-replOptions _ = [ option [] ["repl-options"] "use this option for the repl" id
-              const (reqArg "FLAG" (succeedReadE (:[])) id) ]
+replOptions :: ShowOrParseArgs -> [OptionField ReplOptions]
+replOptions _ =
+  [ option [] ["repl-no-load"]
+    "Disable loading of project modules at REPL startup."
+    replOptionsNoLoad (\p flags -> flags { replOptionsNoLoad = p })
+    trueArg
+  , option [] ["repl-options"]
+    "use this option for the repl"
+    replOptionsFlags (\p flags -> flags { replOptionsFlags = p ++ replOptionsFlags flags })
+    (reqArg "FLAG" (succeedReadE (:[])) id)
+  ]

Note that what previously was id and const now became replOptionsFlags and a lambda (\p flags -> ...). This is a getter and setter pair however previously the setter was not updating the old flags value. That's because ultimately during option parsing the appending of options will take place at https://github.com/haskell/cabal/blob/master/Cabal/src/Distribution/Simple/Command.hs#L159 (notice the mappend).

So the short summary is that previously there were no ++ and it shouldn't have been added in the 785ea62 commit.

[Mikolaj]

@sergv: thank you for the analysis and confirmation. I think the merge is overdue. Thank you all!

[sirlensalot]

Sorry for the delay responding, but obviously I concur with it as the author of the bug! Clearly my tests didn't catch this.

Indeed my only comment would have been: coverage? Can we get the bugfix covered by a unit test. I wrote a test for my functionality, which clearly didn't cover so I'm assuming this broke other workflows. Can we get coverage for those in a follow-on PR please?

[sirlensalot]

Happy to help if somebody can give the use-case clearly

[bacchanalia]

I though this would be covered by the tests in the other PR (#7851) I was working on, but due to code changes in that pr the tests no longer cover this, so we do still need tests for it.