Cabal Error Refactor #9018

SuganyaAK · 2023-06-12T21:49:29Z

Fixes #8618 #8543

This PR introduces three major changes

A datatype CabalException for the Cabal package that has just enough constructors to cover a few modules I have committed as part of this PR, which will incrementally grow and a future idea of creating CabalInstallException for Cabal-Install Package.
A function dieWithException which will replace die' function and throws Exceptions.
A datatype VerboseException which will be polymorphic over CabalException and CabalInstallException

I would appreciate your feedback, review comments and suggestions.

To identify/differentiate cabal errors, I used "C=" as suggested in issue Integration with the error message index? #8543.
For the start, I used random numbers for error codes. I plan to Index these error codes and document it, I request suggestions or directions on this.

Please include the following checklist in your PR:

[✅ ] 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!

…' calls and throws Error types as exception. CabalException will hold all ErrorTypes, types will be incrementally added per module. The VerboseException a will have CabalException and cabalInstallException variously in the a position Utils.hs 1. Creation of Error data types 2. Diewithexception and exceptionCode function 3. Instance for VerboseException Bench.hs and install.hs 1. Replaced die' call sites with dieWithException Right now I have only added errors from two modules Distribtuion/Simple/Bench and Distribution/Simple/Install. Error types will be added incrementally.

ulysses4ever · 2023-06-13T00:10:06Z

One immediate objection that comes to mind: let's not put more stuff in Utils.hs and instead have a separate Errors module?

ulysses4ever · 2023-06-13T00:21:09Z

More generally, CI runs with -Werror, which means that in order to get a useful feedback from it you should make sure that your code compiles without warnings.

Also, we format the code base with fourmolu-0.12. Install it and run make style to make sure you adhere to its rules...

SuganyaAK · 2023-06-15T20:02:59Z

One immediate objection that comes to mind: let's not put more stuff in Utils.hs and instead have a separate Errors module?

Yes I agree. It will be easy to refer to the exception types as well.
errors.hs or exceptions.hs which will be a better module name u think? and creating in Distribution.Simple will make sense right?

ulysses4ever · 2023-06-15T21:28:39Z

I don't think the name or location matter all that much: we can always bike-sched it. I'd look what others do (e.g. Stack) or just go with Distribution.Simple.Errors.

gbaz · 2023-06-15T22:29:10Z

This is a good start. You'll eventually want a version of withMetadata that gets the callStack from VerboseException passed in -- or to inline that function into the display instance.

Also, I think the idea is that each type of exception gets its own constructor with its own code. So each individual BenchmarkException etc. should have a constructor eventually, rather than one catchall constructor which takes a string. Then there should be a function exceptionToMessage or the like that maps each exception constructor to the rendered string.

…type

SuganyaAK · 2023-06-23T12:35:08Z

This is a good start. You'll eventually want a version of withMetadata that gets the callStack from VerboseException passed in -- or to inline that function into the display instance.

Also, I think the idea is that each type of exception gets its own constructor with its own code. So each individual BenchmarkException etc. should have a constructor eventually, rather than one catchall constructor which takes a string. Then there should be a function exceptionToMessage or the like that maps each exception constructor to the rendered string.

Thank you, I have created a function exceptionWithMetada which takes in the callStack from VerboseException and made a few changes to the code as it already sets AlwaysMark and VerboseTrace.

The new module errors.hs has the constructors for Cabal Exceptions , functions exceptionCode(renders error codes) and exceptionMessage(renders error messages).

On the idea of each type of exception having its own constructor, I have incorporated the same in the modules bench.hs and install.hs.
However in the install.hs module, I have grouped a few dieWithException calls to one constructor CompilerNotInstalled as the errors are similar. Please let me know is this makes sense.

…' calls and throws Error types as exception. CabalException will hold all ErrorTypes, types will be incrementally added per module. The VerboseException a will have CabalException and cabalInstallException variously in the a position Utils.hs 1. Creation of Error data types 2. Diewithexception and exceptionCode function 3. Instance for VerboseException Bench.hs and install.hs 1. Replaced die' call sites with dieWithException Right now I have only added errors from two modules Distribtuion/Simple/Bench and Distribution/Simple/Install. Error types will be added incrementally.

…type

…abal into Cabal-Error-Refactor

Cabal/src/Distribution/Simple/Utils.hs

Cabal/src/Distribution/Simple/Errors.hs

This file is redundant.

Cabal/src/Distribution/Simple/Errors.hs

Cabal/src/Distribution/Simple/Utils.hs

ulysses4ever · 2023-07-07T14:11:31Z

One more thing: there's a "Manual QA process": https://github.com/haskell/cabal/blob/master/CONTRIBUTING.md#qa-notes It'd be best if in the original post in the PR under the heading "QA Notes" you put instructions on how to create a simple project that fails and prints a message with an error code.

… to the test

gbaz · 2023-07-10T23:32:41Z

The "rip" changes look good, but note the new failures.

Also I think you are printing callstacks at too low a verbosity -- note that there were no callstacks printed in these circumstances before.

SuganyaAK · 2023-07-11T15:47:10Z

The "rip" changes look good, but note the new failures.

Also I think you are printing callstacks at too low a verbosity -- note that there were no callstacks printed in these circumstances before.

I see for all the test cases that are failed in this run, the marked verbose input is "-vverbose +markoutput +nowrap". I think the recent failures that popped up, happened after I replaced the die' call sites with new constructors in the Glob.hs . I could see this with the error code that shows. ` Unfortunately all these tests passed on my local, so I couldn't see this coming.

gbaz · 2023-07-14T15:18:34Z

I believe this is basically ready for review, and it would be good to rapidly review and merge so that other work can proceed along the same lines on top of this basic exception/reporting structure...

geekosaur · 2023-07-14T15:24:32Z

Cabal/src/Distribution/Simple/BuildPaths.hs

@@ -192,7 +192,8 @@ getSourceFiles verbosity dirs modules = flip traverse modules $ \m ->
    findFileWithExtension ["hs", "lhs", "hsig", "lhsig"] dirs (ModuleName.toFilePath m)
      >>= maybe (notFound m) (return . normalise)
  where
-    notFound module_ = die' verbosity $ "can't find source for module " ++ prettyShow module_
+    notFound module_ =
+      dieWithException verbosity $ CantFindSoureModule module_


OOC, why is it spelled "Soure" instead of "Source"?

It's a typo?

Suggested change

dieWithException verbosity $ CantFindSoureModule module_

dieWithException verbosity $ CantFindSourceModule module_

Yes it was a typo, Thanks for pointing out. Changed to "CantFindSourceModule"

Kleidukos

Nicely packed first PR for an ambitious goal, thank you very much @SuganyaAK

Cabal/src/Distribution/Simple/Errors.hs

Cabal/src/Distribution/Simple/Utils.hs

SuganyaAK · 2023-07-16T22:47:05Z

Nicely packed first PR for an ambitious goal, thank you very much @SuganyaAK

Thank you

Nicely packed first PR for an ambitious goal, thank you very much @SuganyaAK

Thank you @Kleidukos

Kleidukos

LGTM, thanks!

ulysses4ever · 2023-07-20T17:14:41Z

@mergify refresh

mergify · 2023-07-20T17:14:44Z

refresh

✅ Pull request refreshed

ulysses4ever added type: enhancement re: error-message Concerning error messages delivered to the user labels Jun 13, 2023

ulysses4ever requested a review from gbaz June 13, 2023 00:18

SuganyaAK added 3 commits June 13, 2023 17:48

Codebase is formatted with fourmolu and compiled without warnings.

fe970bc

Enabled fourmolu action

722d778

Running fourmolu through github action as the previous ones failed.

574bf67

Function exceptionWithMetadata and constructors for each exception …

5663f3b

…type

SuganyaAK added 5 commits June 23, 2023 09:51

Codebase is formatted with fourmolu and compiled without warnings.

570d1e5

Enabled fourmolu action

6842366

Running fourmolu through github action as the previous ones failed.

3f11d92

Function exceptionWithMetadata and constructors for each exception …

9e6a92c

…type

SuganyaAK force-pushed the Cabal-Error-Refactor branch from 5663f3b to 9e6a92c Compare June 23, 2023 13:51

SuganyaAK added 2 commits June 23, 2023 09:53

Updated changelog

efac6a4

Merge branch 'Cabal-Error-Refactor' of https://github.com/SuganyaAK/c…

56a58ac

…abal into Cabal-Error-Refactor

gbaz reviewed Jun 23, 2023

View reviewed changes

Cabal/src/Distribution/Simple/Utils.hs Show resolved Hide resolved

gbaz reviewed Jun 23, 2023

View reviewed changes

Cabal/src/Distribution/Simple/Utils.hs Outdated Show resolved Hide resolved

PatternMatching cases of diewithexception and displayexception reduced

9c90d0b

ulysses4ever reviewed Jun 23, 2023

View reviewed changes

Cabal/src/Distribution/Simple/Errors.hs Outdated Show resolved Hide resolved

SuganyaAK added 2 commits June 23, 2023 16:06

Delete fourmolu.yaml

56e4473

This file is redundant.

Delete settings.json

b47370b

This file is redundant.

ulysses4ever reviewed Jun 24, 2023

View reviewed changes

Cabal/src/Distribution/Simple/Errors.hs Outdated Show resolved Hide resolved

ulysses4ever reviewed Jun 24, 2023

View reviewed changes

Cabal/src/Distribution/Simple/Utils.hs Outdated Show resolved Hide resolved

SuganyaAK added 3 commits July 7, 2023 13:58

Accepted the new output with error codes and adding the file relevant…

d47b8ae

… to the test

Changes in Utils.hs and Errors.hs to reflect the new error codes

2fe47db

Keeping rip intact

2ad952d

gbaz and others added 4 commits July 12, 2023 13:09

test changes

bbc63bc

CallStack unmarked from the error output

cf5a38f

callstack unmarked from output

972eb33

unmarked callstack in test output

12a5846

ulysses4ever added the attention: needs-review label Jul 14, 2023

ulysses4ever marked this pull request as ready for review July 14, 2023 15:25

geekosaur approved these changes Jul 14, 2023

View reviewed changes

Kleidukos requested changes Jul 14, 2023

View reviewed changes

Cabal/src/Distribution/Simple/Errors.hs Outdated Show resolved Hide resolved

Cabal/src/Distribution/Simple/Errors.hs Outdated Show resolved Hide resolved

Cabal/src/Distribution/Simple/Utils.hs Outdated Show resolved Hide resolved

SuganyaAK added 3 commits July 14, 2023 20:58

Format the codebase with fourmolu

2fd3503

Changed to Error identifier to "Cabal-"

9aa458c

Updated Constructors as per review comments

2dc8828

Rectifying Hlint warning

4fd1225

SuganyaAK requested a review from Kleidukos July 17, 2023 17:45

Kleidukos approved these changes Jul 17, 2023

View reviewed changes

gbaz added squash+merge me Tell Mergify Bot to squash-merge and removed attention: needs-review labels Jul 18, 2023

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

gbaz merged commit 667be46 into haskell:master Jul 20, 2023

SuganyaAK deleted the Cabal-Error-Refactor branch August 3, 2023 02:31

SuganyaAK mentioned this pull request Aug 7, 2023

Successive Cabal Package Errors #9162

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cabal Error Refactor #9018

Cabal Error Refactor #9018

SuganyaAK commented Jun 12, 2023 •

edited

Loading

ulysses4ever commented Jun 13, 2023

ulysses4ever commented Jun 13, 2023

SuganyaAK commented Jun 15, 2023

ulysses4ever commented Jun 15, 2023

gbaz commented Jun 15, 2023

SuganyaAK commented Jun 23, 2023

ulysses4ever commented Jul 7, 2023

gbaz commented Jul 10, 2023

SuganyaAK commented Jul 11, 2023

gbaz commented Jul 14, 2023

geekosaur Jul 14, 2023

ulysses4ever Jul 14, 2023

geekosaur Jul 16, 2023

SuganyaAK Jul 16, 2023

Kleidukos left a comment

SuganyaAK commented Jul 16, 2023

Kleidukos left a comment

ulysses4ever commented Jul 20, 2023

mergify bot commented Jul 20, 2023

	dieWithException verbosity $ CantFindSoureModule module_
	dieWithException verbosity $ CantFindSourceModule module_

Cabal Error Refactor #9018

Cabal Error Refactor #9018

Conversation

SuganyaAK commented Jun 12, 2023 • edited Loading

ulysses4ever commented Jun 13, 2023

ulysses4ever commented Jun 13, 2023

SuganyaAK commented Jun 15, 2023

ulysses4ever commented Jun 15, 2023

gbaz commented Jun 15, 2023

SuganyaAK commented Jun 23, 2023

ulysses4ever commented Jul 7, 2023

gbaz commented Jul 10, 2023

SuganyaAK commented Jul 11, 2023

gbaz commented Jul 14, 2023

geekosaur Jul 14, 2023

Choose a reason for hiding this comment

ulysses4ever Jul 14, 2023

Choose a reason for hiding this comment

geekosaur Jul 16, 2023

Choose a reason for hiding this comment

SuganyaAK Jul 16, 2023

Choose a reason for hiding this comment

Kleidukos left a comment

Choose a reason for hiding this comment

SuganyaAK commented Jul 16, 2023

Kleidukos left a comment

Choose a reason for hiding this comment

ulysses4ever commented Jul 20, 2023

mergify bot commented Jul 20, 2023

✅ Pull request refreshed

SuganyaAK commented Jun 12, 2023 •

edited

Loading