Add test for compilation error capture

[sorki]

Looks like capturing compilation errors only works with some GHCs. Adding a test to try in CI.

[sorki]

Looks like it works in hint-0.9.0.4 (and also in deprecated 0.9.0.5) no matter the GHC, probably a regression.

[sorki]

Bisecting leads to 9df3041

If I revert that one it works again.

[sorki]

Seems to pass with latest version, PTAL.

[gelisam]

Thanks!

That commit is about configuring the logger, so it makes sense that this commit would affect whether the errors are captured or not. It does so in a shim, whose purpose is to allow the callers to run the same code regardless of the GHC version, while the implementation of the shim does something different depending on the GHC version. So it makes sense that the behaviour is different depending on the GHC version.

But that commit message also says that it fixes using unsafeRunInterpreterWithArgs with -package-db. Do we have a test for that, and does it pass with all the supported GHC versions when the commit is reverted?

And if so, do we understand why? The commit message explains exactly why the change fixes unsafeRunInterpreterWithArgs with -package-db. It would be nice to have are explanation for why reverting the commit fixes the log messages, and why the commit is no longer needed for unsafeRunInterpreterWithArgs.

[gelisam]

Good news, we do have a test: https://github.com/haskell-hint/hint/blob/main/unit-tests/run-unit-tests.hs#L353

[sorki]

Thanks!

That commit is about configuring the logger, so it makes sense that this commit would affect whether the errors are captured or not. It does so in a shim, whose purpose is to allow the callers to run the same code regardless of the GHC version, while the implementation of the shim does something different depending on the GHC version. So it makes sense that the behaviour is different depending on the GHC version.

I see!

And if so, do we understand why? The commit message explains exactly why the change fixes unsafeRunInterpreterWithArgs with -package-db. It would be nice to have are explanation for why reverting the commit fixes the log messages, and why the commit is no longer needed for unsafeRunInterpreterWithArgs.

I don't propose reverting the commit since error capture works fine again for GHC9.4+, maybe I can make the test guarded to run only for 9+, since there's already a functionality to determine GHC version in a testsuite, would that work for you?

I'm slowly grokking the hints codebase so I might help with some stuff since I'm using it quite a lot already, but I don't think supporting (or like backporting fixes like this) to old GHC versions is worth it.

Might be related to #149

[gelisam]

maybe I can make the test guarded to run only for 9+, since there's already a functionality to determine GHC version in a testsuite, would that work for you?

A test which prevents a regression for future GHC versions does seem better than no test at all, but I think it's worth trying to understand what is going on and finding a way to make the test pass on all supported GHC versions.

since error capture works fine again for GHC9.4+,

What about GHC9.2? Like I said, the implementation of the Hint.GHC.modifyLogger shim behaves differently based on the GHC version:

• On GHC 9.4, it delegates to GHC.modifyLogger
• On GHC 9.2, it also delegates to GHC.modifyLogger
• On GHC 9.0, it modifies GHC's dynamic flags directly, because the GHC API did not yet have a GHC.modifyLogger function

I would thus expect the behaviour to be different for GHC9.2+, not GHC9.4+!

If the behaviour indeed changes for GHC9.4+, then that makes the problem much more difficult, because we'll have to look in ghc's source code to figure out why the same GHC.modifyLogger behaves differently in ghc-9.2 and ghc-9.4.

I'm slowly grokking the hints codebase

Please let me know if you have any questions! We could even setup a video call if you think that would be more helpful.

so I might help with some stuff

Yes please! I struggle to find the time to give hint the attention it deserves.

but I don't think supporting (or like backporting fixes like this) to old GHC versions is worth it.

hint used to support much older versions of GHC, and support for older versions did get dropped over time in order to ease the maintenance burden. Currently the oldest supported GHC version is ghc-8.6.5, which was released in 2019. I think the standard in the Haskell community is 3 years, so I think hint should support ghc-9.0, which was released in 2021. I think you are suggesting that the oldest GHC we should support is ghc-9.4? That version was released in 2022, now 2 years ago, not that far away from the standard, so that might be fine.