Add the -dynamic flag and update build instructions #2668

pepeiborra · 2022-01-31T22:07:04Z

Addresses some of the issues with building HLS locally flagged in #2659

Reasons to add the -dynamic flag in the haskell-language-server.cabal descriptor:

cabal install haskell-language-server --enable-executable-dynamic doesn't work as it doesn't actually link the dynamic rts. Note that cabal build --enable-executable-dynamic works fine, so this is an issue with cabal install.
cabal install haskell-language-server --ghc-option=-dynamic works, but it applies the dynamic flag to all the dependencies, which takes really long as it plays badly with the Cabal store. It will also fail if the user has enabled profiling builds since GHC doesn't ship with a profiling dynamic rts.

So adding the -dynamic flag in the Cabal descriptor really is the only way to get what we want.

I was reluctant to do this previously since -dynamic might not work in all the supported operating systems, e.g. Windows. We also need to disable the flag when building static binaries for releases.

hasufell · 2022-01-31T22:13:31Z

How does a user disable this flag now?

pepeiborra · 2022-01-31T22:15:06Z

How does a user disable this flag now?

I added a Cabal flag that we can use for release builds. Why would a user need to disable this flag though?

michaelpj · 2022-01-31T22:27:32Z

Can we preserve the reasoning from the PR description somewhere? Perhaps in the cabal file where the flag is defined? This stuff is quite fiddly and if we don't write it down we'll forget why we're doing what we're doing.

hasufell · 2022-01-31T22:27:46Z

How does a user disable this flag now?

I added a Cabal flag that we can use for release builds. Why would a user need to disable this flag though?

I don't know. But assumptions often explode into one's face, so better prepare for an exit strategy 😅

michaelpj

I haven't been following the discussion enough to know if this is actually the right thing to do 😅

michaelpj · 2022-01-31T22:28:03Z

.github/workflows/build.yml

@@ -48,6 +48,11 @@ jobs:
        echo "tests: false" >> cabal.project.local
        echo "benchmarks: false" >> cabal.project.local

+    - name: Disable -dynamic


Why do we disable it here?

Release builds are static (makes distribution easier), but we tell users to build dynamic. ..it's all quite confusing 😄

Afaiu static builds work fine as long as you use an official GHC bindist (does it work across ubuntu vs fedora bindists?)...otherwise TH may break.

We want to build static binaries for ease of distribution. Sadly static binaries are no good for loading code dynamically, which is needed for compiling TH code.

At this point tbh i am not sure if makes sense provide static binaries for linux/macOS if we are asking users build from source, showing a warning every time they open hls, which only can be removed doing it.
Given how extended is the use of TH in the ecosystem, only very beginners and ejem exotic code bases :-P are not using it.

Well i suppose we can keep them with the hope a better solution comes from ghc sooner or later.

Afaiu static builds work fine as long as you use an official GHC bindist (does it work across ubuntu vs fedora bindists?)...otherwise TH may break.

Yeah i guess those are the users who had th working but have to close the warning or build hls from source without needing it.

jneira

sorry but I am not sure about this, it breaks the default build for windows and windows users must disable the flag explicitly to make it work

so at very least I would ask to make the flag only applicable in non windows os via a cabal condition if !os(windows)

otoh at this point I am not sure if this is needed for sure to get a hls binary which works for TH

are we 100% sure a binary dinamically linked in the target os only against system libs (the default link mode in Linux!) does not work? maybe simply build hls in the target os is the key, as we are compiling exactly with same ghc used in the projects

the fact we did not get to reproduce the th issue in CI makes very difficult get a solution

so I think it worths to investigate a little bit more the issue before enable this which could open a new whole kind of issues

jneira · 2022-02-01T06:41:45Z

This was suggested in this comment: #2659 (comment)

So adding the -dynamic flag in the Cabal descriptor really is the only way to get what we want.

Not sure if can trust cabal but maybe there would be another way, adding to a cabal.project or cabal.project.local:

package haskell-language-server
  ghc-options: -dynamic

only for linux and macos of course

pepeiborra · 2022-02-01T07:55:25Z

sorry but I am not sure about this, it breaks the default build for windows and windows users must disable the flag explicitly to make it work
so at very least I would ask to make the flag only applicable in non windows os via a cabal condition if !os(windows)

The only reason I didn't disable it for Windows initially is because I wanted to see CI fail. The Cabal condition is added now.

otoh at this point I am not sure if this is needed for sure to get a hls binary which works for TH

are we 100% sure a binary dinamically linked in the target os only against system libs (the default link mode in Linux!) does not work? maybe simply build hls in the target os is the key, as we are compiling exactly with same ghc used in the projects

The goal with -dynamic is to get the rts_thr_dyn flavour, so that dynamic code loading uses the system linker. I don't know how many times I need to say this.

the fact we did not get to reproduce the th issue in CI makes very difficult get a solution

so I think it worths to investigate a little bit more the issue before enable this which could open a new whole kind of issues

I think your concerns are unfounded. We are simply aligning HLS with GHC:

$ ghc +RTS --info                                                            
 [("GHC RTS", "YES")
 ,("GHC version", "8.10.7")
 ,("RTS way", "rts_thr_dyn")

I suppose what you are asking for is a repro. It shouldn't be too difficult to create one just by going through the myriad of existing TH issues.

jneira · 2022-02-01T08:47:36Z

The only reason I didn't disable it for Windows initially is because I wanted to see CI fail. The Cabal condition is added now.

Nice, many thanks.

I suppose what you are asking for is a repro. It shouldn't be too difficult to create one just by going through the myriad of existing TH issues.

Yeah it would make our steps towards the solution in a more confident way for sure.
Adding such repro, make it fail with a default hls binary and work with the dynamically linked one would be really great

addressed

jneira

Ok this is clearly a improvement over the actual situation and make build instructions clearer.
Moreover it makes the build work for cabal and stack.

But it is a workaround over a cabal bug and a stack limitation (stack would need a package entry in the stack,yaml with ghc-options) and it should not be needed, as already commented in the flag (well the cabal part)

I really hope this will not open the door to other bugs, but it has been tested in stack succesfully, where --ghc-options="dynamic" failed.

Linux is not my primary os and my knowledge of linux is limited (even more than windows :-P) so i am gonna trust my co-maintainers living in linux

Thanks!

pepeiborra · 2022-02-01T14:01:53Z

@jneira can we bump the version number of haskell-language-server.cabal and upload it to Hackage?

jneira · 2022-02-02T08:33:36Z

@Mergifyio backport 1.6.1.1-hackage

* Add the -dynamic flag and update build instructions * Disable dynamic in release build * tweak wording * add a comment * Disable dynamic on Windows (cherry picked from commit 3f12824)

mergify · 2022-02-02T08:34:01Z

backport 1.6.1.1-hackage

✅ Backports have been created

#2682 Add the -dynamic flag and update build instructions (backport #2668) has been created for branch 1.6.1.1-hackage

* Add the -dynamic flag and update build instructions * Disable dynamic in release build * tweak wording * add a comment * Disable dynamic on Windows (cherry picked from commit 3f12824) Co-authored-by: Pepe Iborra <pepeiborra@gmail.com>

Related to haskell#2668. This fixs the build, however, because now HLS is dynamicly linked, it pulls GHC as a dependency. The uncompressed closure size is now `~6GiB`.

* nix: ghc92: ignore broken plugins * nix: use current ghc for tools in the shell Theses tools are pulled in the shell, but we don't need them to match the GHC version used for development. Said otherwise, as long as we use a working `cabal-install` to build with GHC 9.2, we don't care if that cabal-install was built with GHC 8.10. This gives more chance for theses tools to work and be in the binary cache of nixpkgs. * nix: disable shake-bench for ghc921 * nix: fix ghc 9.2 shell * nix: explicit cabal project in devShell Using alias, we get the "correct" behavior when typing `cabal build` in the nix-shell, it points to the current cabal-project file. * nix: remove special case for different ghc versions * nix: move implicit-hie-cradle as a flake input * nix: restore dev shell This commit restores a working behavior for `nix-shell` or `nix develop`, for all supported GHC versions. When entering a `nix-shell`, or `nix develop .#haskell-language-server-ghcXxX-dev` you will get an environment with all the dependencies needed to run `cabal build`. Haskell dependencies will be built by cabal instead of nix. This may be more robust compared to a shell where dependencies are built by nix: - It won't try to build any dependency with nix. This mean that it won't benefit from nix caching, but on the other hand, it will perfectly match the cabal plan. - The nix shell may fail to start if an (possibly unneeded) dependency was failing to build. Entering nix-shell also generates an alias command `cabal_project` which runs cabal with the correct project file if needed (e.g. for GHC 9.0 and 9.2). Users are notified with a message when entering the shell. The old behavior (i.e. where dependencies are built with nix) is still available with `nix develop .#haskell-language-server-xxx-dev-nix` (i.e. suffix the name of the shell with `-nix`). * nix: New entries which build all target at once All HLS version can be built with `nix build .#all-haskell-language-server`, and all development shells with `nix build .#all-dev-shells`. I had to workaround a limitation in `nix build` which is not able to build multiples targets, instead I used linkFarmFromDrvs. All packages and shells are now named with a unique package name containing the GHC version. * nix: Build all devShells and all binaries in CI * nix: build HLS with dynamic linking Related to #2668. This fixs the build, however, because now HLS is dynamicly linked, it pulls GHC as a dependency. The uncompressed closure size is now `~6GiB`. * nix; fix CI command * nix: only build dev shell with nix packages for current GHC version * Another tentative to fix the build * fix: Eval and alternate number format works with ghc 9.2 * fix CI build * ANother tentative fix

…2668)" This reverts commit 3f12824.

pepeiborra requested review from jneira and michaelpj as code owners January 31, 2022 22:07

pepeiborra requested review from Ailrun and Anton-Latukha as code owners January 31, 2022 22:14

pepeiborra added 3 commits January 31, 2022 22:15

Add the -dynamic flag and update build instructions

44273a8

Disable dynamic in release build

a79b7a5

tweak wording

64f0e26

pepeiborra force-pushed the dynamic branch from e8068b6 to 64f0e26 Compare January 31, 2022 22:15

michaelpj reviewed Jan 31, 2022

View reviewed changes

add a comment

6d8f812

jneira previously requested changes Feb 1, 2022

View reviewed changes

jneira mentioned this pull request Feb 1, 2022

Issues around th warning (This HLS binary does not support Template Haskell) and build hls with dynamic linking #2659

Closed

Disable dynamic on Windows

331e55b

jneira self-requested a review February 1, 2022 08:47

jneira linked an issue Feb 1, 2022 that may be closed by this pull request

Build dynamic executables by default on supported platforms #2245

Closed

jneira approved these changes Feb 1, 2022

View reviewed changes

jneira merged commit 3f12824 into master Feb 1, 2022

jneira mentioned this pull request Feb 2, 2022

Prepare 1.6.1.1 (only hackage release) #2681

Merged

mergify bot mentioned this pull request Feb 2, 2022

Add the -dynamic flag and update build instructions (backport #2668) #2682

Merged

jneira mentioned this pull request Feb 2, 2022

Nix flake fix dev shells #2655

Merged

jneira added a commit to jneira/haskell-language-server that referenced this pull request Feb 28, 2022

Revert "Add the -dynamic flag and update build instructions (haskell#…

451a9e6

…2668)" This reverts commit 3f12824.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the -dynamic flag and update build instructions #2668

Add the -dynamic flag and update build instructions #2668

pepeiborra commented Jan 31, 2022 •

edited

Loading

hasufell commented Jan 31, 2022

pepeiborra commented Jan 31, 2022

michaelpj commented Jan 31, 2022

hasufell commented Jan 31, 2022

michaelpj left a comment

michaelpj Jan 31, 2022

hasufell Jan 31, 2022 •

edited

Loading

pepeiborra Feb 1, 2022

jneira Feb 1, 2022

jneira Feb 1, 2022

jneira left a comment

jneira commented Feb 1, 2022

pepeiborra commented Feb 1, 2022 •

edited

Loading

jneira commented Feb 1, 2022

jneira left a comment •

edited

Loading

pepeiborra commented Feb 1, 2022

jneira commented Feb 2, 2022

mergify bot commented Feb 2, 2022

Add the -dynamic flag and update build instructions #2668

Add the -dynamic flag and update build instructions #2668

Conversation

pepeiborra commented Jan 31, 2022 • edited Loading

hasufell commented Jan 31, 2022

pepeiborra commented Jan 31, 2022

michaelpj commented Jan 31, 2022

hasufell commented Jan 31, 2022

michaelpj left a comment

Choose a reason for hiding this comment

michaelpj Jan 31, 2022

Choose a reason for hiding this comment

hasufell Jan 31, 2022 • edited Loading

Choose a reason for hiding this comment

pepeiborra Feb 1, 2022

Choose a reason for hiding this comment

jneira Feb 1, 2022

Choose a reason for hiding this comment

jneira Feb 1, 2022

Choose a reason for hiding this comment

jneira left a comment

Choose a reason for hiding this comment

jneira commented Feb 1, 2022

pepeiborra commented Feb 1, 2022 • edited Loading

jneira commented Feb 1, 2022

jneira left a comment • edited Loading

Choose a reason for hiding this comment

pepeiborra commented Feb 1, 2022

jneira commented Feb 2, 2022

mergify bot commented Feb 2, 2022

✅ Backports have been created

pepeiborra commented Jan 31, 2022 •

edited

Loading

hasufell Jan 31, 2022 •

edited

Loading

pepeiborra commented Feb 1, 2022 •

edited

Loading

jneira left a comment •

edited

Loading