Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prepare 0.22.9 release #449

Merged
merged 1 commit into from
Jul 31, 2024
Merged

Prepare 0.22.9 release #449

merged 1 commit into from
Jul 31, 2024

Conversation

hsyl20
Copy link
Contributor

@hsyl20 hsyl20 commented Jul 30, 2024

No description provided.

@hsyl20 hsyl20 mentioned this pull request Jul 30, 2024
@hsyl20 hsyl20 force-pushed the hsyl20/hide-error-links branch from 1f4a73a to cc6ec65 Compare July 30, 2024 10:04
@hsyl20
Copy link
Contributor Author

hsyl20 commented Jul 30, 2024

Somehow cabal-doctest doesn't find the new version of doctest. Isn't it because we pass --ignore-project?

@sol
Copy link
Owner

sol commented Jul 31, 2024

Somehow cabal-doctest doesn't find the new version of doctest. Isn't it because we pass --ignore-project?

Yes, that's expected. The cabal-doctest workflow will fail whenever you do a release. Build failures for cabal-coctest are informational for that reason, they don't block a merge. I don't have a better solution for this yet.

@hsyl20 hsyl20 force-pushed the hsyl20/hide-error-links branch from bdac283 to 531c3b0 Compare July 31, 2024 08:29
@hsyl20
Copy link
Contributor Author

hsyl20 commented Jul 31, 2024

This MR now includes the fix for the testsuite to pass. See #448 (comment)

@sol
Copy link
Owner

sol commented Jul 31, 2024

With the right command line to run tests (I was using cabal test) I only get 1 failure with 9.10:

It's still not clear to me why this failure does not show up on CI. Is this maybe depending on the value of TERM or something? On the one hand, this would make sense, on the other hand you shouldn't look at TERM at all if the output is not going to a terminal.

@sol
Copy link
Owner

sol commented Jul 31, 2024

on the other hand you shouldn't look at TERM at all if the output is not going to a terminal.

The reason why ghci prints those links in the first place is because of

evalThrow interpreter "GHC.IO.Handle.hDuplicateTo System.IO.stdout System.IO.stderr"

Redirection of stderr is only done after ghci has started. At that point GHC already decided that output is going to a terminal, hence we see the links.

A proper solution would be to do something along the lines of: https://github.com/hspec/sensei/blob/5c11026fa48e13ea1c351ab882765eb0966f2e97/src/Language/Haskell/GhciWrapper.hs#L63-L71

But things are actually even more complicated than that (not elaborating on it now, but if you want to work on it then please ask and I'll give the full story). That's why we are going with #448 for now.

src/GhcUtil.hs Outdated
-- failures like this:
-- expected: "Foo.hs:6:1: error: [GHC-58481]\n parse error..."
-- but got: "Foo.hs:6:1: error: [\ESC]8;;https://errors.haskell.org/messages/GHC-58481\ESC\\GHC-58481\ESC]8;;\ESC\\]\n parse error..."
let dynflags = dynflags' { useErrorLinks = Never }
Copy link
Owner

@sol sol Jul 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hsyl20 we need to differentiate between:

  1. Extracting comments with the GHCAPI.
  2. Evaluating expressions with GHCi.

For (1) we want the exact same error behavior as GHC, including colors and hyperlinks.

For (2) we don't want colors and hyperlinks.

This code is concerned with (1), so we actually don't want this changes.

For the related test failure, this is actually an order dependency somehow. If you focus the single test, then that test succeeds. You could be anywhere, including in GHC itself. I don't have the time to investigate further right now. It doesn't happen on CI, so releases are not blocked by it.

Copy link
Owner

@sol sol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please include only the version bump in this PR. (see my other comments)

@hsyl20 hsyl20 force-pushed the hsyl20/hide-error-links branch from 531c3b0 to f447f74 Compare July 31, 2024 14:38
@hsyl20
Copy link
Contributor Author

hsyl20 commented Jul 31, 2024

I've removed the other commit. I won't have much time to work on this soon either.

@sol sol merged commit 6d21787 into sol:main Jul 31, 2024
47 of 51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants