Skip to content

Comments

Fix brig doc module generation#1975

Merged
pcapriotti merged 3 commits intodevelopfrom
pcapriotti/fix-brig-cabal
Dec 10, 2021
Merged

Fix brig doc module generation#1975
pcapriotti merged 3 commits intodevelopfrom
pcapriotti/fix-brig-cabal

Conversation

@pcapriotti
Copy link
Contributor

Follow-up to #1956, fixing issues reported by @fisx:

  • remove obsolete data-files stanza from brig's cabal file;
  • generate doc modules on repl invocation, as well as on build.

No CHANGELOG entry.

Checklist

  • The PR Title explains the impact of the change.
  • The PR description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.

@fisx
Copy link
Contributor

fisx commented Dec 8, 2021

No CHANGELOG entry.

You could manually add the two PRs to the one changelog entry you probably (hopefully?) made about the change you're fixing here?

fisx
fisx previously approved these changes Dec 8, 2021
Copy link
Contributor

@fisx fisx left a comment

Choose a reason for hiding this comment

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

I still get the same error, but I'm not sure how to reproduce it without a 200 year old emacs haskell-mode and a self-written ghci wrapper script. stack repl works fine.

But... how?! And why did having the data file in the cabal file break things?

Copy link
Contributor

Choose a reason for hiding this comment

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

don't files that are required for compilation also count as data files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so.

Copy link
Contributor

Choose a reason for hiding this comment

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

but they should make it into the tar ball created by cabal sdist, no? and that's only happening if they are listed here.

$ cabal sdist
remote: Enumerating objects: 60, done.
remote: Counting objects: 100% (60/60), done.
remote: Compressing objects: 100% (25/25), done.
remote: Total 60 (delta 40), reused 55 (delta 35), pack-reused 0
Unpacking objects: 100% (60/60), 14.09 KiB | 721.00 KiB/s, done.
From https://github.com/wireapp/http2
 * [new branch]      aeson-2             -> origin/aeson-2
 * [new branch]      client-status-fixme -> origin/client-status-fixme
 * [new branch]      preface-race        -> origin/preface-race
 * [new branch]      smatting/debugging  -> origin/smatting/debugging
Previous HEAD position was 7c465be Better support for setting header table size to 0
HEAD is now at 1ee1ce4 Make sure connection preface is always sent first
cabal: Error: Could not find module: Brig.Docs.Swagger with any suffix:
["gc","chs","hsc","x","y","ly","cpphs","hs","lhs","hsig","lhsig"]. If the
module is autogenerated it should be added to 'autogen-modules'.

ah, the last line is interesting!

Copy link
Contributor

Choose a reason for hiding this comment

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

(yes, we don't do sdists, but we might as well pretend! :))

Copy link
Contributor

Choose a reason for hiding this comment

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

and why did you modify the cabal file instead of package.yaml?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This only ended up by mistake in the cabal file, not in the package.yaml file.

@fisx fisx self-requested a review December 8, 2021 20:21
@fisx fisx dismissed their stale review December 8, 2021 20:25

changed my mind

@fisx
Copy link
Contributor

fisx commented Dec 8, 2021

concourse failed for the same reason #1964 failed earlier today:

  test/integration/Test/Federator/InwardSpec.hs:77:9: 
  1) Federator.API.Inward should be able to call cargohold
       uncaught exception: ErrorCall
       Assertions failed:
        1: 403 =/= 500
       
       Response was:
       
       Response {responseStatus = Status {statusCode = 500, statusMessage = "Internal Server Error"}, responseVersion = HTTP/1.1, responseHeaders = [("Transfer-Encoding","chunked"),("Date","Wed, 08 Dec 2021 13:56:38 GMT"),("Server","Warp/3.3.17"),("Content-Type","application/json")], responseBody = Just "{\"code\":500,\"message\":\"Federation is not yet implemented for this endpoint\",\"label\":\"federation-not-implemented\"}", responseCookieJar = CJ {expose = []}, responseClose' = ResponseClose}
       CallStack (from HasCallStack):
         error, called at src/Bilge/Assert.hs:89:5 in bilge-0.22.0-FhngfA5b3MW5Tg8nd5Ja7l:Bilge.Assert
         <!!, called at src/Bilge/Assert.hs:107:19 in bilge-0.22.0-FhngfA5b3MW5Tg8nd5Ja7l:Bilge.Assert
         !!!, called at test/integration/Test/Federator/InwardSpec.hs:77:9 in main:Test.Federator.InwardSpec

is it possible that #1956 or #1973 broke develop?

@pcapriotti
Copy link
Contributor Author

I still get the same error, but I'm not sure how to reproduce it without a 200 year old emacs haskell-mode and a self-written ghci wrapper script. stack repl works fine.

If you can share a script that breaks, I'm happy to look into it. But both stack repl and cabal repl work for me.

But... how?! And why did having the data file in the cabal file break things?

No, that was unrelated, just some harmless leftover. The fix is to generate the doc files on the repl hook, as well as on the build hook.

@pcapriotti pcapriotti force-pushed the pcapriotti/fix-brig-cabal branch from 6ef4326 to 8a8ea22 Compare December 9, 2021 15:01
@pcapriotti pcapriotti force-pushed the pcapriotti/fix-brig-cabal branch from 8a8ea22 to 730c196 Compare December 10, 2021 09:55
@pcapriotti pcapriotti merged commit 9d4bebf into develop Dec 10, 2021
@pcapriotti pcapriotti deleted the pcapriotti/fix-brig-cabal branch December 10, 2021 11:05
@sysvinit sysvinit mentioned this pull request Dec 10, 2021
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