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

hls-stan-plugin shouldn't be included by default #4464

Open
KovalevDima opened this issue Dec 9, 2024 · 12 comments
Open

hls-stan-plugin shouldn't be included by default #4464

KovalevDima opened this issue Dec 9, 2024 · 12 comments
Labels
bindists Anything to do with binary distributions of HLS os: nixos type: support User support tickets, questions, help with setup etc.

Comments

@KovalevDima
Copy link

KovalevDima commented Dec 9, 2024

There are several reasons to distribute HLS sources without Stan included by default:

  1. Stan still marked as beta after a 4 years of development.
  2. Stan has a large dependency footprint.
  3. The Stan plugin is disabled by default in text editor plugins.
  4. If you want to disable it as a dependency, you must rebuild and redistribute HLS.

As an HLS user, I encountered problems while trying to modify the HLS binary distribution with Nix due to the lack of support for Stan dependencies (extension library). I’ve been having conversations with several active Haskell open-source contributors who also disable Stan entirely.

What is the history behind enabling Stan by default? Is it necessary to invest time into adopting a tool with limited support?

Mark this issue with 👍 if you think (or 👎 if not) the hls-stan-plugin should not be enabled by default. Share any stories about why you disable stan

@KovalevDima KovalevDima added status: needs triage type: bug Something isn't right: doesn't work as intended, documentation is missing/outdated, etc.. labels Dec 9, 2024
@fendor fendor added os: nixos bindists Anything to do with binary distributions of HLS type: support User support tickets, questions, help with setup etc. and removed status: needs triage type: bug Something isn't right: doesn't work as intended, documentation is missing/outdated, etc.. labels Dec 9, 2024
@fendor
Copy link
Collaborator

fendor commented Dec 10, 2024

Thanks for opening this issue!

Pinging the maintainer of the hls-stan-plugin @0rphee, and the maintainer of stan @tomjaguarpaw.

What is the history behind enabling Stan by default?

For clarification, this is about shipping HLS binaries with hls-stan-plugin support by default, as the plugin is, as you point out, disabled by default. I.e., opt-in for users who want to use it.

The history is essentially, we accept most plugin requests, as we don't have a policy regarding stability or even regular maintenance (iirc).
hls-stan-plugin was removed in a prior release due to the lack of GHC support, but then new maintainers added the plugin again and have been receptive to HLS issues so far (even if there weren't any 😛 ).

I’ve been having conversations with several active Haskell open-source contributors who also disable Stan entirely.

Personally, I also disable hlint, and know more than enough Haskell devs who do not use formatters for various reasons.
I don't feel like this is a compelling argument.

Is it necessary to invest time into adopting a tool with limited support?

Ironically, hls-stan-plugin (and for that matter stan) has not been a source of issues or time investments for HLS developers, iirc.

It feels like this issue is describing issues downstream packagers (in particular nixpkgs) have with their respective Haskell package maintenance. Perhaps there are other ways as well to ease the burden, e.g. add extensions to Stackage. Afaict, it should just be a matter of adding extensions to stackage, as there are no dependencies that aren't already on Stackage.

@tomjaguarpaw
Copy link
Member

I could benefit from some clarification here. Firstly, @KovalevDima says

distribute HLS sources without Stan included

but @fendor says

shipping HLS binaries with hls-stan-plugin support by default

What exactly is under discussion here? What exactly is the proposed remediation?

And what is the alternative? For users who do want to use the Stan plugin, how could they continue to use it if it were "not included by default" (whatever that means)?

@Bodigrim
Copy link
Contributor

My understanding is that the suggestion is essentially to flip

flag stan
description: Enable stan plugin
default: True

so that it's False by default.

Could distributors such as nixpkgs flip the flag themselves? I imagine they do not need an explicit consent of HLS team to do that.

@KovalevDima
Copy link
Author

Thank you for your answers

It's possible with Nix to drop stan as a dependency. However, my point is not about Nix. It's just an example from my experience when I tried to test unreleased HLS updates (support of multiple-home-units) and run into a problem with stan's trash dependencies

My point was about HLS plugins distribution and stan especially. I understand that experienced HLS developers are already accustomed to rebuilding all of the HLS plugins and adapting to the Kowainik ecosystem. However, regular HLS users shouldn’t have to deal with the Kowainik ecosystem when they want to test new features or adapt their environment to work with HLS on fresh GHC builds. It's not always obvious - even to experienced developers that you can simply override the HLS stan flag to prevent half of the Kowainik packages from being built.

I'm one of those developers who doesn't love to use formatters, but I understand and know people who do. At the same time, I’ve never met anyone who uses stan in a commercial project, especially with HLS. Also it's so bad that static analyzer tool requires:

  1. relude custom prelude library 🤡
  2. CSS preprocessor clay
  3. toml serialization config library tomland (while almost all of haskell tools uses yaml)
  4. "very opinionated library" (official caution) slist

There are also some libraries that easy to avoid from use in static analyzer tool. But stan requires them by default.

My goal is to share some of my experiences working with HLS. I appreciate the HLS team's work, and I’m glad to have used HLS every day for more than three years, with all its problems and benefits. Special thanks to @fendor for the progress on multiple-home-units support, which has made my personal life as a monorepo enthusiast better.

@KovalevDima
Copy link
Author

And what is the alternative? For users who do want to use the Stan plugin, how could they continue to use it if it were "not included by default" (whatever that means)?

In my opinion, there should be a way in HLS to link plugins dynamically without modifying the source code and redistributing it. However, I understand that it might be difficult to implement.

Regarding my suggestion: users of the hls-stan-plugin should rebuild HLS themselves by passing the stan flag to Cabal, which should default to False:

flag stan 
   description: Enable stan plugin 
   default:     False
   manual:      True

My position here is biased, as I don’t know any users of stan, and I think stan is more of a curious concept with a poor implementation

@0rphee
Copy link
Collaborator

0rphee commented Dec 12, 2024

Thanks for the ping.

I don't have any strong opinions either way, though I sympathize with @ KovalevDima's complaints. If stan's codebase was smaller, I may had volunteered to split the display-related code from the stan library to remove dependencies such as clay, however, I'm afraid that other libraries like relude would still be part of the build...

@KovalevDima
Copy link
Author

@0rphee, thank you for your understanding!
stan was primarily designed as a CLI tool. What if we merge the parts of its codebase that HLS depends on into the hls-stan-plugin and then start refactoring it to reduce the dependency footprint? I can invest my time in this.

In my opinion, the concept behind stan’s HIE file analysis is a good one. However, I believe it would be much better to implement something like an HIE file analysis SDK within HLS, allowing users to create their own HLS plugins

@0rphee
Copy link
Collaborator

0rphee commented Dec 19, 2024

@KovalevDima That seems like a nice idea. However, since the diagnostic infrastructure, etc., is already in stan, I think it would be a better approach to refactor that logic into a package that could be used as the backend for both stan and a hls plugin, specially since we would get a taste of what the api would need to look like, as it would be used by stan from the start.

I'm open to help with this, if you're interested, we could discuss a plan to do this on the stan repo, that is, if @tomjaguarpaw would be open to accept changes of this sort in stan itself.

@michaelpj
Copy link
Collaborator

@KovalevDima I'm uclear why "build HLS with the stan flag disabled" isn't an acceptable solution for you?

  • The existence of the flag lets people choose if they want it or not
  • Having stan enabled has not (yet) been a problem for our main distributions of HLS, so I don't see why we should change the default: it seems like the problem occurs less commonly and there is already a way to fix it

That said: I do think there is a case for stan being problematic, and I think that's probably about its dependency footprint. Probably we should be trying to keep HLS's dependency footprint down, and that suggests not accepting plugins that add a lot to it. Of course, this is in tension with having it be full-featured. So I don't think there's a clear rule here. But especially given that we don't enable the stan plugin by default (since many people really dislike its default hints and don't care to configure it), it's questionable for us to spend our dependency footprint points on something that most of our users aren't using. So maybe we should consider removing it again unless it either a) becomes useful to all our users by default or b) becomes very slim.

@Bodigrim
Copy link
Contributor

So I don't think there's a clear rule here.

Ideally HLS should be available from Stackage, in which case a rule could be "all plugin dependencies available from Stackage".

@michaelpj
Copy link
Collaborator

Ideally HLS should be available from Stackage

Should it be? It's an application, not a library, so it's not clear that it needs to fit into a nice consistent package set like Stackage provides. You can e.g. always build HLS from the repository using our own index-state and get a good build - you don't need Stackage to ensure that everything plays well with your own application's dependencies.

@Bodigrim
Copy link
Contributor

That's why I said "ideally", not "necessary". I imagine if you want to package a dynamically linked HLS, say, for Ubuntu, you'd like it to be consistent with the rest of system packages, which are usually derived from Stackage set.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bindists Anything to do with binary distributions of HLS os: nixos type: support User support tickets, questions, help with setup etc.
Projects
None yet
Development

No branches or pull requests

6 participants