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

1.2.0 hackage release fails to build due to last lsp-types release #2087

Closed
jneira opened this issue Aug 10, 2021 · 10 comments
Closed

1.2.0 hackage release fails to build due to last lsp-types release #2087

jneira opened this issue Aug 10, 2021 · 10 comments
Labels
CI Continuous integration old_type: distribution type: bug Something isn't right: doesn't work as intended, documentation is missing/outdated, etc..

Comments

@jneira
Copy link
Member

jneira commented Aug 10, 2021

  • Reported by an user in discord
Failed to build hls-plugin-api-1.1.0.2.
Build log (
/home/nick/.cabal/logs/ghc-8.10.4/hls-plugin-api-1.1.0.2-c11c62afa2a5742cc1e627b0988a64b31813f6a7c3b9e899e22661d8fd7c7c51.log
):
Configuring library for hls-plugin-api-1.1.0.2..
Preprocessing library for hls-plugin-api-1.1.0.2..
Building library for hls-plugin-api-1.1.0.2..
[1 of 6] Compiling Ide.Logger       ( src/Ide/Logger.hs, dist/build/Ide/Logger.o, dist/build/Ide/Logger.dyn_o )
[2 of 6] Compiling Ide.Plugin.Config ( src/Ide/Plugin/Config.hs, dist/build/Ide/Plugin/Config.o, dist/build/Ide/Plugin/Config.dyn_o )
[3 of 6] Compiling Ide.Plugin.Properties ( src/Ide/Plugin/Properties.hs, dist/build/Ide/Plugin/Properties.o, dist/build/Ide/Plugin/Properties.dyn_o )
[4 of 6] Compiling Ide.Types        ( src/Ide/Types.hs, dist/build/Ide/Types.o, dist/build/Ide/Types.dyn_o )

src/Ide/Types.hs:235:33: error:
    Ambiguous occurrence ‘length’
    It could refer to
       either ‘Prelude.length’,
              imported from ‘Prelude’ at src/Ide/Types.hs:17:8-16
              (and originally defined in ‘Data.Foldable’)
           or the field ‘length’,
              imported from ‘Language.LSP.Types’ at src/Ide/Types.hs:47:1-35
              (and originally defined in ‘lsp-types-1.3.0.1:Language.LSP.Types.SemanticTokens’)
           or the field ‘length’,
              imported from ‘Language.LSP.Types’ at src/Ide/Types.hs:47:1-35
              (and originally defined in ‘lsp-types-1.3.0.1:Language.LSP.Types.SemanticTokens’)
    |
235 |             (_, []) -> (limit - length xx, it)
    |                                 ^^^^^^
cabal: Failed to build hls-plugin-api-1.1.0.2 (which is required by
exe:haskell-language-server from haskell-language-server-1.2.0.0 and
exe:haskell-language-server-wrapper from haskell-language-server-1.2.0.0). See
the build log above for details.

We have to make a hackage revision to use the previous lsp-types version

@jneira jneira added type: bug Something isn't right: doesn't work as intended, documentation is missing/outdated, etc.. CI Continuous integration old_type: distribution labels Aug 10, 2021
@jneira
Copy link
Member Author

jneira commented Aug 10, 2021

  • the hackage revision will be needed for hls-plugin-api and maybe ghcide, hopefully plugins will get the correct version transitively (see Build with lsp 1.2.0.1 #2059)
  • that pr should be backported to 1.3.0-hackage

@jneira
Copy link
Member Author

jneira commented Aug 11, 2021

The cause of this breakage has been:

  • 2021-07-01 ghcide-1.4.0.3 was released relaxing the lsp-types bounds from 1.2.* to >=1.2 && <1.4 (with an optimistic minor patch version with the actual info should have been a major one)
  • 2021-07-31 lsp-types-1.3.0.0 was released with a breaking change (bumping up correctly the version following pvp), adding record fields which collides with prelude length and other defintions as _start
  • hls-plugin-api depends only on lsp but lsp-1.2.0.1 minor patch version was released, bumping the lsp-types bound to ==1.3.* and introduding transitively the mentioned breaking changes (so lsp should have been 1.3.0.0 too following pvp iiuc)

So the solution could be make hackage revision over actual lastest hackage versions:

  • restore in ghcide-1.4.0.3 the lsp-types bound to == 1.2.* and set lsp to == 1.2.0.0
  • set the lsp bound in hls-plugin-api to == 1.2.0.0

@pepeiborra what was the motivation to relax the lsp-types bound to < 1.4? i guess it was to being able to get lsp-1.2.0.1, could be revert that change as a hackage revision? If that is not possible we'll have to restrict ghcide version everywhere to == 1.4.0.2 (it will force a build including ghcide and hls-plugin-api to get lsp-1.2.0.0)

EDIT: Arguably adding those new fields should not break dependent packages if they would imported lsp qualified, so following strictly pvp a minor bump was correct.

@pepeiborra
Copy link
Collaborator

#1985

@jneira
Copy link
Member Author

jneira commented Aug 11, 2021

Hmm, thanks for the link to the pr changing it, however i was curious about the hackage release itself too (not mentioned in the pr). To know if we could go back to lsp-types-1.2.* in ghcide-1.4.0.3 (as the code with CPP allows it)

@jneira
Copy link
Member Author

jneira commented Aug 11, 2021

Alternatively we could backport #2059 to a tag, branch or commit of ghcide-1.4.0.3 and hls-plugin-api-1.1.0.2 and release ghcide-1.4.0.4 and hls-plugin-api-1.1.0.3

@jneira
Copy link
Member Author

jneira commented Aug 11, 2021

And to avoid this situation hereinafter and let lsp* add new definitions without bumping up the breaking change version we should import lsp modules qualified everywhere.
Although after experiencing this isssue i guess add fields with names wich collides with widely used prelude definitions was not a good idea (needed to integrate with aeson without much customization?) 😟

@pepeiborra
Copy link
Collaborator

Hmm, thanks for the link to the pr changing it, however i was curious about the hackage release itself too (not mentioned in the pr). To know if we could go back to lsp-types-1.2.* in ghcide-1.4.0.3 (as the code with CPP allows it)

Thanks for checking. Yes, I think it's fine to go back to lsp-types-1.2.* in this case since I ended up rolling back the lsp-types hashconsing patch in our Hackage snapshot too.

My opinion is that this trouble has been caused by the change to relax the lsp-types upper bound, which was too optimistic, and we shouldn't read too much into it (I.e. no need to make all imports qualified). Maybe a Hackage revision would suffice.

Thanks for looking into it @jneira !

@jneira
Copy link
Member Author

jneira commented Aug 11, 2021

My opinion is that this trouble has been caused by the change to relax the lsp-types upper bound, which was too optimistic

Yep, but lsp-1.2.0.1 would break the build anyways (as ghcide would ask for lsp == 1.2.* and lsp-types == 1.2.* but lsp-1.2.0.1 for lsp-types == 1.3.*)

@jneira
Copy link
Member Author

jneira commented Aug 11, 2021

I've done the both revisions:

I've tested locally both builds succesfully and hls-1.2.0 find a build plan (still building)
So it should be fixed

@jneira jneira closed this as completed Aug 11, 2021
@LinuxUser404
Copy link

Thanks, the issue is fixed ))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous integration old_type: distribution type: bug Something isn't right: doesn't work as intended, documentation is missing/outdated, etc..
Projects
None yet
Development

No branches or pull requests

3 participants