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

Bumping retrie version #2347

Closed
Tarmean opened this issue Nov 12, 2021 · 16 comments · Fixed by #2513
Closed

Bumping retrie version #2347

Tarmean opened this issue Nov 12, 2021 · 16 comments · Fixed by #2513

Comments

@Tarmean
Copy link

Tarmean commented Nov 12, 2021

Retrie currently doesn't work on windows, which also breaks the renaming support of haskell-language-server #2051

A pull request to fix this was merged a while ago facebookincubator/retrie#31 , and this fixed the issue for me when pinning the commit in extra-deps/source-repository-package. However as far as I can see the commit isn't part of any retrie release.

I'd quite like to get windows support for retrie into haskell-language-server. The easy answer would be to pin the commit, but doing so in every build variant seems a bit roundabout. Is there a more proper way to go about this?

@jneira
Copy link
Member

jneira commented Nov 12, 2021

Finally good news! thanks for fixing it upstream and noting it. I guess the more straightforward solution would be get the fix released, maybe @pepeiborra could help us in doing so.

In the meanwhile add the source-repository-package could help as the plugin is broken anyways, at least we would get a github release with it fixed

@jneira
Copy link
Member

jneira commented Nov 12, 2021

However i am bit perplexed with the relation between hls-rename-plugin and retrie. I would swear renaming has been working for me in windows with the actual retrie version, and i see the hls-rename-plugin tests are being executed in windows and the test suite pass 🤔 nvm, the plugin is importing retrie modules but i guess it is using auxiliar functionality not affected by the xargs bug.

In any case i suppose it would fix the hls-retrie-plugin itself. Afaics we have no test suite for that plugin so not sure what would be the actual situation.

@pepeiborra
Copy link
Collaborator

I can push out a retrie minor release this weekend or next week if that helps.

@jneira
Copy link
Member

jneira commented Nov 12, 2021

I can push out a retrie minor release this weekend or next week if that helps.

please, that way we will not have to do temporary config changes

@jneira jneira added the status: blocked Not actionable, because blocked by upstream/GHC etc. label Nov 12, 2021
@pepeiborra
Copy link
Collaborator

pepeiborra commented Nov 12, 2021

Since this will be a minor version bump in retrie, it shouldn't block the HLS 1.5 release

@Tarmean
Copy link
Author

Tarmean commented Nov 12, 2021

~However i am bit perplexed with the relation between hls-rename-plugin and retrie.

Apparently the prototype used Retrie but the current version does it directly with SYB and exactprint, sorry for the confusion
https://gist.github.com/OliverMadine/96927f88b6e5e7890e5179559089166c

A minor retrie release seems ideal, thank you for working on this so quickly!

@pepeiborra
Copy link
Collaborator

retrie-1.1.0.0 is up in Hackage (it was a major release according to @kowainik policeman)

@jneira
Copy link
Member

jneira commented Nov 14, 2021

thsnks @pepeiborra, @Tarmean would you have any chance to bump the version used here? will do myself if you have not

@Tarmean
Copy link
Author

Tarmean commented Nov 22, 2021

Does the CI run the tests for all build versions? I'm mildly worried because in most builds still use retrie-0.1.1.1. Since there aren't any transitive dependencies on retrie I don't see how the change could break anything, though.

@jneira
Copy link
Member

jneira commented Nov 22, 2021

we have the hackage index fixed at some point of time in cabal.project, and the retrie version fixed in all stack.yamls so some changes are needed to make the builds pick up the new version

@Tarmean
Copy link
Author

Tarmean commented Nov 22, 2021

Stack and cabal seem to build fine, but the nix expression in the CI build fails https://github.com/Tarmean/haskell-language-server/runs/4289543574?check_suite_focus=true

I definitely do not know enough nix to figure out how all that fits together. After staring at the nix code for half an hour my guess is that the pinned hackage version is too old? https://github.com/NixOS/nixpkgs/blob/nixos-unstable/pkgs/data/misc/hackage/pin.json

@jneira
Copy link
Member

jneira commented Nov 22, 2021

Maybe our hls nix hacker could help with that: @berberman

@jneira jneira added this to the 1.6.0 milestone Dec 15, 2021
@jneira
Copy link
Member

jneira commented Dec 15, 2021

@Tarmean hi! could you open a pr here?, i think we could fix the nix issue with some help from maintainers using nix

@jneira jneira removed the status: blocked Not actionable, because blocked by upstream/GHC etc. label Dec 15, 2021
@jneira jneira mentioned this issue Dec 20, 2021
@jneira
Copy link
Member

jneira commented Dec 20, 2021

ok retrie-1.2.0.0 only is available for ghc>=9.2 and there is no upper bound in any .cabal file so hls for ghc-9.2.0 will pick it automatically
To have the fix for ghc >= 8.8 and <= 9.0.1 we would need a retrie-1.1.0.1 with the patch included and I am not sure if retrie maintainers do backports.

/cc @pepeiborra @Tarmean

@pepeiborra
Copy link
Collaborator

What patch are you referring to @jneira? I thought that retrie 1.1.0.0 is good for Windows

@jneira
Copy link
Member

jneira commented Dec 20, 2021

ouch sorry, 1.1.0.0 already has the patch, yeah
so it should being used for cabal builds with the > 8.6.5 and it will be used for stack with my recent pr #2513
unfortunately we cant enforce the patched version in .cabal files while we support 8.6.5

thanks and sorry for the confusion, will make that #2513 close this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants