Deprecating --solver=modular CLI argument

[yvan-sraka]

EDIT: This PR DOES modify cabal behavior (remove --solver=modular CLI argument). During a pair-programming session with @andreabedini, we noticed that the current chooseSolver function (which returns an IO Solver) unnecessarily complicates the code. Currently, the only available option is indeed the Modular solver.

Checklist:

• Patches conform to the coding conventions.

[Kleidukos]

Ah we need more pair programming sessions if we're getting nice refactorings like this, thank you @andreabedini for taking the time

[yvan-sraka]

I extended the PR with more lines removed 85ebc06 ... It has the side effect of removing this (useless) CLI option:

% cabal build --solver=test
Error: cabal: invalid argument to option `--solver': solver must be one of:
modular

[andreabedini]

My only condern is about parsing the config files. Can one specify the solver in cabal.project? What about cabal.config?

I admit it's very unlikely that anybody ever did but I would prefer to avoid a sudden parsing error.

[andreabedini]

Same thing for the cli option. We could leave the option and just make it emit an informative warning/error.

[yvan-sraka]

Then I can just discard my second commit and create another PR that add deprecation warnings for the --solver CLI arg (add .cabal attribute?!), in the same fashion that what I've done here.

[grayjay]

Thanks for cleaning up the solver option! I agree that we should deprecate the option before completely removing it.

[andreabedini]

I am not sure a fully deprecating cycle makes sense here, the option never really did anything useful since there always been only one option to choose from!

My concern is about failing on parse, which would be very suprising for the unlucky developer who had the idea of fixing the value of solver: for future reproducibility :)

To be extra clear, I suggest both --solver= and solver: emit a warning saying the option is deprecated and suggesting to just stop using it (there's no migration needed). This is done at pasing and cli level.

For the rest the PR looks alright already.

[grayjay]

Yes, the option has had no effect since #3598, so we can deprecate it and still remove most of the code that deals with choosing a solver.

[andreabedini]

I think there is reasonable consensus here.

@yvan-sraka can you add a tests showing that we can still parse configs that specify solver: and that we give a warning.

[Kleidukos]

Same thing for QA notes, we'd need a couple of steps to reproduce the graceful handling of the solver: option after this PR. :)

[yvan-sraka]

I added warnings in docs and for CLI --solver option, but I may need pointers to where and how to add warnings is the .cabal format parser (within cabal-install/src/Distribution/Client/ProjectConfig/ I guess?).

[andreabedini]

Few things are left to do:

• Warning for cabal build --solver=modular
• Warning for solver: modular in the config file. I have checked and the file will still parse, giving an "unrecognised field" warning.
• The initial config still includes -- solver: modular
• Tests that codify this behavior.

I think I am ok with an "unrecognised field" warning to be honest. If you want to add a proper warning you can start from here

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

Line 1325 in 6f5a1f1

-- TODO: next step, make the deprecated fields elicit a warning.

😂

[andreabedini]

I think configSolver in ConfigExFlags and anything downstream of that can go. The fact that we keep the option does not mean we need to keep its value (expecially given there's never been any value!).

[andreabedini]

@yvan-sraka Is there anything here that wasn't included in #9282? It looks like we end up keeping configSolver in ConfigExFlags. Why?

[yvan-sraka]

@yvan-sraka Is there anything here that wasn't included in #9282?

No! This PR included everything that is in #9282 + it removes --solver=modular CLI argument.

It looks like we end up keeping configSolver in ConfigExFlags. Why?

I believe that what I aimed to do was to deprecate it with a warning message rather than completely removing it. But thanks for spotting it. I've added a new commit that completely removes the option from the CLI.

[andreabedini]

I believe that what I aimed to do was to deprecate it with a warning message rather than completely removing it.

As far as I remember the plan was this #9206 (review).

With "the fact that we keep the option does not mean we need to keep its value" I meant something like this:

, optionSolver configSolver (\_ flags -> {- maybe print a warning with unsafePerformIO? -} flags)

[yvan-sraka]

Few things are left to do:

• Warning for cabal build --solver=modular
• Warning for solver: modular in the config file. I have checked and the file will still parse, giving an "unrecognised field" warning.
• The initial config still includes -- solver: modular
• Tests that codify this behavior.

I still need to find where the initial config is defined: I found only it in test(s)/IntegrationTests2/(nix-)config/default-config ... Was your suggestion about updating those tests or about creating a new one?

I think I am ok with an "unrecognised field" warning to be honest. If you want to add a proper warning you can start from here

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

Line 1325 in 6f5a1f1

-- TODO: next step, make the deprecated fields elicit a warning.

😂

TBH, this would be a bit ambitious for my actually parsec skills, but I can open a follow-up issue on that :)

With "the fact that we keep the option does not mean we need to keep its value" I meant something like this:

, optionSolver configSolver (\_ flags -> {- maybe print a warning with unsafePerformIO? -} flags)

That's exactly what I did:

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

Lines 934 to 940 in 499dc9b

	, optionSolver
	configSolver
	( \_ flags ->
	unsafePerformIO $ do
	hPutStrLn stderr "[WARNING] The --solver flag is deprecated and will be removed in a future release."
	return flags
	)

[yvan-sraka]

I've made a few updates to the PR: PreSolver is no longer an instance of Pretty, and so I inlined the "modular" value. This confirms my tests showing that no configuration template generated by Cabal contains the solver option. And now, if this option is used, the parser show a deprecation warning to the user!

However, I'm still not satisfied by the use of unsafePerformIO. @andreabedini suggested, and has begun working on, a generic and safe way to deprecate any flag / option. I'm uncertain whether it should be included in this PR or be part of a follow-up. WDYT?