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

Merge plugins into the HLS package #3976

Merged
merged 39 commits into from
Jan 29, 2024
Merged

Merge plugins into the HLS package #3976

merged 39 commits into from
Jan 29, 2024

Conversation

michaelpj
Copy link
Collaborator

This is a POC for merging all the plugin packages into the HLS package, as discussed in #3556.

I just did the first two plugins alphabetically to show what it looks like, it's pretty mechanical.

To reiterate, the advantages are:

  • Fewer packages to upload
  • Less repeated metadata
  • Fewer versions
  • Ability to use common stanzas to e.g. have the same set of warnings everywhere (I would do this as a follow up if we decide to go this route)

Things seem to still work fine. The main annoyances are:

  • Can't cd into a plugin directory and cabal build/cabal test
  • Can't cabal test <plugin-name>, instead need cabal test <plugin-name>-tests

I think overall it's still going to be a pretty big improvement

@soulomoon
Copy link
Collaborator

soulomoon commented Jan 17, 2024

One huge draw back is that it prevent us from working on a single plugin along opening just the plugin dir.
From developing perspective, working on a relatively smaller code base is much easier.

I am wondering is there a workaround for this?

@michaelpj
Copy link
Collaborator Author

I don't think it does? You can still just build the component for the plugin and it won't build anything else.

@soulomoon
Copy link
Collaborator

But we can no longer open an single editor containing just one plugin dir and get hls support 🤔

@michaelpj
Copy link
Collaborator Author

I don't see why not? We do support single components of packages just fine? And it'll be better as the multi-component stuff rolls out (but that's already bad if you try to edit multiple packages today)

@soulomoon
Copy link
Collaborator

I don't see why not? We do support single components of packages just fine? And it'll be better as the multi-component stuff rolls out (but that's already bad if you try to edit multiple packages today)

Thanx for clearing out my concern on this.

@michaelpj
Copy link
Collaborator Author

Another downside: stack supports building packages with internal libraries (the CI run succeeded!), but it doesn't do per-component builds, so the dev experience using stack will probably suck. I'm not sure how much we care about that these days 🤷

@michaelpj michaelpj changed the title POC: merge plugins into the HLS package Merge plugins into the HLS package Jan 24, 2024
@michaelpj michaelpj marked this pull request as ready for review January 24, 2024 22:14
@michaelpj
Copy link
Collaborator Author

No longer a POC!

There is still plenty to do here with cleaning up the now rather monolithic cabal file, making better use of common stanzas etc, but I'm keen to land this before too long since it's very conflict-y.

@michaelpj
Copy link
Collaborator Author

cc @fendor @wz1000 as discussed

@michaelpj
Copy link
Collaborator Author

Follow up todos:

  • Make all components which condition on pedantic use the common stanza for that
  • Add a common stanza for default-language
  • Maybe merge in some more packages. I think only hls-test-utils, everything else is a dependency of ghcide and so needs to stay out for now.
  • Maybe try and organize the cabal file to be a bit more navigable

Copy link
Collaborator

@jhrcek jhrcek left a comment

Choose a reason for hiding this comment

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

I like this approach more than the previous.
Few nitpicks about ghc-options - warnings, but I will work on those anyway in followup PRs, so feel free to ignore.

DataKinds
TypeOperators

ghc-options: -Wno-unticked-promoted-constructors
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can be removed - is now in common warnings.

, apply-refact

cpp-options: -DHLINT_ON_GHC_LIB
ghc-options:
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can import common warnings for this lib and remove this.

@michaelpj michaelpj force-pushed the mpj/one-package branch 2 times, most recently from 2a3755b to 7a064d8 Compare January 28, 2024 20:18
@michaelpj
Copy link
Collaborator Author

It was tasty-rerun, turning it off makes the tests run. I don't know whether it's doing something dodgy, but it seems to be orthogonal to this PR at least.

@michaelpj michaelpj dismissed fendor’s stale review January 29, 2024 09:15

Figured out the issue

@michaelpj michaelpj merged commit 06ec06c into master Jan 29, 2024
31 checks passed
@smunix
Copy link
Contributor

smunix commented Mar 29, 2024

@michaelpj - it looks like this change broke nix rpath setting for plugins internal libraries. It fails during the install_phase as it finds references to the /build/ directory. Has 2.7.0.0 been installed successfully with nix at all?

@michaelpj
Copy link
Collaborator Author

That seems like a question for the Nix Haskell folks? It builds with cabal, so if it doesn't build with the Nix setup then that's a bug in the Nix setup, I think.

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.

7 participants