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

Compatibility with optparse-applicative-0.18 #284

Closed

Conversation

andreasabel
Copy link

@andreasabel andreasabel commented Jun 10, 2023

optparse-applicative-0.18 switched to prettyprinter, so some reexports are missing now.

Building library for xrefcheck-0.2.2..
[17 of 19] Compiling Xrefcheck.CLI    ( src/Xrefcheck/CLI.hs, /Users/abel/bin/src/xrefcheck/dist-newstyle/build/x86_64-osx/ghc-9.2.8/xrefcheck-0.2.2/build/Xrefcheck/CLI.o, /Users/abel/bin/src/xrefcheck/dist-newstyle/build/x86_64-osx/ghc-9.2.8/xrefcheck-0.2.2/build/Xrefcheck/CLI.dyn_o )

src/Xrefcheck/CLI.hs:34:46: error:
    Module ‘Options.Applicative.Help.Pretty’ does not export ‘displayS’
   |
34 | import Options.Applicative.Help.Pretty (Doc, displayS, fill, fillSep, indent, renderPretty, text)
   |                                              ^^^^^^^^

src/Xrefcheck/CLI.hs:34:79: error:
    Module
    ‘Options.Applicative.Help.Pretty’
    does not export
    ‘renderPretty’
   |
34 | import Options.Applicative.Help.Pretty (Doc, displayS, fill, fillSep, indent, renderPretty, text)
   |                                                                               ^^^^^^^^^^^^

src/Xrefcheck/CLI.hs:34:93: error:
    Module ‘Options.Applicative.Help.Pretty’ does not export ‘text’
   |
34 | import Options.Applicative.Help.Pretty (Doc, displayS, fill, fillSep, indent, renderPretty, text)
   |                                                                                             ^^^^
Error: cabal: Failed to build xrefcheck-0.2.2

This PR restores compatibility with the latest optparse-applicative.

I simplified the code to display the help text, but the rendering still looks fine:

To ignore a link in your markdown, include "<!-- xrefcheck: ignore <mode> -->"
comment with one of these modes:
  "link"                   Ignore the link right after the comment.
  "paragraph"              Ignore the whole paragraph after the comment.
  "file"                   This mode can only be used at the top of markdown or
                           right after comments at the top.

So maybe the code was unnecessary complicated to begin with.

On hackage I added upper bound optparse-applicative < 0.18, e.g. https://hackage.haskell.org/package/xrefcheck-0.2.2/revisions/.

@andreasabel
Copy link
Author

What's up with your CI?

@gromakovsky
Copy link
Member

@andreasabel thank you for this PR. We are currently switching from Buildkite to GitHub Actions in #267. I hope we'll finish soon and then CI might start working in this PR after rebasing on the latest master branch.

@andreasabel
Copy link
Author

Thanks for the heads up, @gromakovsky ! I leave further handling of this PR to you, please use/modify it in any way you want.

@@ -768,7 +768,7 @@ checkExternalResource followed config@Config{..} link
other -> throwError $ ExternalResourceSomeError $ show other
where
retryAfterInfo :: Response a -> Maybe RetryAfter
retryAfterInfo = readMaybe . decodeUtf8 <=< L.lookup hRetryAfter . responseHeaders
retryAfterInfo = readMaybe . decodeUtf8 @Text <=< L.lookup hRetryAfter . responseHeaders
Copy link
Contributor

Choose a reason for hiding this comment

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

In this case, readMaybe comes from Text.Read and takes a String parameter, so it does not compile for me unless this type parameter application is removed.

Copy link
Author

Choose a reason for hiding this comment

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

I was surprised that I had to add it, because it is orthogonal to the concern of this PR, namely optparse-applicative.
Let me investigate some more...

Copy link
Author

Choose a reason for hiding this comment

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

So indeed stack build fails now (which uses LTS 19 which is GHC 9.0), but cabal build -w ghc-9.2.8 succeeds and needs this change.
I am gonna remove this patch, it is something to be sorted out when migrating from GHC 9.0 to 9.2 and up.
(Note that GHC 9.2.8 is the currently recommended version of GHC.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it should be updated at some point. I will create an issue referencing this comments thread. Thank you!

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@aeqz aeqz left a comment

Choose a reason for hiding this comment

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

I have tried this code changes on my device while CI is fixed.

After making it build (mentioned in other comment), tests are passing, but part of the CLI help message has to be fixed:

master:
master

This PR:
pr

Maybe we can also add a golden test for this help message.

@andreasabel
Copy link
Author

After making it build (mentioned in other comment), tests are passing, but part of the CLI help message has to be fixed:

I can reproduce it with the stack build, but it does not happen when built with GHC 9.2.8 on cabal.

However, the stack build uses optparse-applicative-0.16 rendering this whole enterprise obsolete (compatibility with 0.18).
I suppose this PR comes too early, should be reconsidered when the build basis has moved on to stack nightly or an LTS that packages 0.18.

@aeqz
Copy link
Contributor

aeqz commented Jun 16, 2023

but it does not happen when built with GHC 9.2.8 on cabal

So all the points that I mentioned seem to be related to the GHC version difference. We will go back to this PR when upgrading the stack setup, probably after finishing the ongoing CI work!

@int-index
Copy link
Member

Not knowing about this PR, I ended up with a similar fix in 74ed941.

So I think all relevant changes are now in master. Feel free to reopen if I missed something.

@int-index int-index closed this Oct 14, 2024
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.

4 participants