-
-
Notifications
You must be signed in to change notification settings - Fork 367
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
Consolidate all cabal.project files #2866
Conversation
f11c688
to
99f0217
Compare
b22ade9
to
d6cdd0c
Compare
439c689
to
cf05dee
Compare
.github/workflows/test.yml
Outdated
@@ -113,13 +113,6 @@ jobs: | |||
ghc: ${{ matrix.ghc }} | |||
os: ${{ runner.os }} | |||
|
|||
# repeating builds to workaround segfaults in windows and ghc-8.8.4 | |||
- name: Build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously we could have had plugins which built on GHC X (and hence weren't commented out in the special cabal.project
), but whose tests didn't pass (and hence weren't included in the testing workflow. These plugins would have been built by this step, which would have ensured that they kept compiling, even if the tests didn't pass.
Now I think we will only build stuff that's necessary for the tests that we try and run, so if we don't test a plugin we also won't ensure that it keeps building.
Also I think we won't check compilation of the main HLS executable either?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good observation, thanks!
I deleted this step before figuring out that we can use the buildable
field to selectively disable projects per ghc version. With that trick, hopefully this step can stay and we can avoid all the pitfalls that you discovered
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a feeling that that won't work. I think I suggested this to @jneira in the past and he convinced me that it wouldn't work. I can't remember why though >:(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't see the reason why it wouldn't work. CI is failing because the Cabal plan it computes is wrong for 9.0.1, but I get the right plan locally so it might be a CI bug
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think maybe it had something to do with Cabal not taking buildable
into account when making a plan, so it won't be clever enough to try setting the splice
flag to false
if the splice plugin isn't buildable, it'll just fail. Worth checking though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't need to be:
haskell-language-server/haskell-language-server.cabal
Lines 237 to 239 in 4b475b0
if flag(haddockComments) && (impl(ghc < 9.2.1) || flag(ignore-plugins-ghc-bounds)) | |
build-depends: hls-haddock-comments-plugin ^>= 1.0 | |
cpp-options: -DhaddockComments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, so you're going to keep the existing machinery? Maybe the thing that wouldn't work was a more ambitious thing: I'd though that maybe we could automatically have plugins be enabled or disabled depending on whether the plugin package was buildable given the rest of the configuration or not. So then we'd only have to put the logic for deciding buildability in the plugin package, which would be nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One step at a time..
ee5d6d8
to
77cca7b
Compare
152307f
to
a65466a
Compare
a65466a
to
acf3603
Compare
@michaelpj the Nix CI is failing, but I am going to leave it to you (after this PR has landed). Deal? |
if impl(ghc >= 9.2) | ||
buildable: False | ||
else | ||
buildable: True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not ideal, afaict, this spreads disabling plugins over multiple files, but it looks like we have a hard time doing better than this.
Is haskell/cabal#7783 good enough to allow us to centralise this again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know whether it's good enough, but I agree that buildabilty with ghc x.y would sit better in the project file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually think it's better in the plugin files! "Does this plugin work on GHC X" feels like a concern of the plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, awesome!
It's already broken, you're not making it any worse! |
-haddockComments | ||
-retrie | ||
-splice | ||
-tactic, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the retrie, splice, and tactics plugins also get buildable
conditionals?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, how is the CI passing?? Did they just start working without us noticing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, it's because we still have the GHC conditionals for inclusion in haskell-language-server.cabal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah you are right - I'm not sure how cabal build
decides what to build, so far I'm just adding the buildable
conditionals on demand
Consolidate all the various project files into a single one that aggregates all the
allow-newer
andconstraints
from the existing project files.cabal.project
cabal-ghc90.project
cabal-ghc92.project
This is possible now that our dependencies (stylish-haskell, hlint, etc.) have moved forward to
ghc-lib
9.2 and that all the per-ghc-version cabal flags have been eliminated (#2867).