Skip to content

nix: no more yolo on ghc 9.4; test suites generally work! #3373

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

Closed
wants to merge 6 commits into from

Conversation

lf-
Copy link
Contributor

@lf- lf- commented Dec 2, 2022

I tried using HLS again in one of my projects and found that the nix packaging
isn't possible to use with other projects due to overriding mkDerivation
and causing world-rebuilds. (work vendored most of the HLS flake
because of this but I want to actually fix it upstream)

It turns out that the GHC 9.4 set is stable enough to remove the yolo anyway.

I've also added an overlay attribute that uses the nixpkgs set without
the other overrides to mkDerivation, for use as an overlay in other flakes.

@lf-
Copy link
Contributor Author

lf- commented Dec 2, 2022

hls-cabal-plugin> src/Ide/Plugin/Cabal/Parse.hs:21:1: error:
hls-cabal-plugin>     Ambiguous module name ‘Distribution.Types.Version’:
hls-cabal-plugin>       it was found in multiple packages:
hls-cabal-plugin>       Cabal-3.6.3.0 Cabal-syntax-3.8.1.0

how does that even happen?! There are no mentions of Cabal-syntax anywhere I can find and my checking finds it is version 3.6, with version 3.8.1.0 as a separate version there.

@lf-
Copy link
Contributor Author

lf- commented Dec 2, 2022

Ohhhh. Gross. It's nixpkgs.

  # 2022-10-27: implicit-hie 0.1.3.0 needs a newer version of Cabal-syntax.
  implicit-hie = super.implicit-hie.override {
    Cabal-syntax = self.Cabal-syntax_3_8_1_0;
  };

@lf-
Copy link
Contributor Author

lf- commented Dec 2, 2022

Yeah the build was jacked before I touched it, that explains a lot 😇

Explanation of the eldritch nix horrors that have been invoked here: what happens when you override boot packages is "you can't". So you can't override cabal to be newer without forcing the package set to be inconsistent and have multiple versions of the same package wind up in a package database and cause build failures. That is just what happens if you override a boot package. Nix/cabal goes all zalgo-text on you :(

@lf- lf- requested a review from fendor as a code owner December 2, 2022 06:42
Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

Changes to the cabal plugin look good to me!

@michaelpj michaelpj added the merge me Label to trigger pull request merge label Dec 2, 2022
@michaelpj michaelpj enabled auto-merge (squash) December 2, 2022 10:17
@michaelpj michaelpj disabled auto-merge December 2, 2022 14:49
@michaelpj
Copy link
Collaborator

Hmm, the Nix CI jobs are still failing, would you expect them to succeed?

Copy link
Collaborator

@guibou guibou left a comment

Choose a reason for hiding this comment

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

Great.

Note for the future: it is possible to change the YOLO thing so the jailbreak and doCheck are only added if the package is broken. This improves a lot the caching.

@lf-
Copy link
Contributor Author

lf- commented Dec 2, 2022

Hmm, the Nix CI jobs are still failing, would you expect them to succeed?

nope, they were broken before i changed anything

@lf-
Copy link
Contributor Author

lf- commented Dec 2, 2022

Great.

Note for the future: it is possible to change the YOLO thing so the jailbreak and doCheck are only added if the package is broken. This improves a lot the caching.

this is actually done already in flake.nix :) so the yolo is unnecessary

@lf-
Copy link
Contributor Author

lf- commented Dec 4, 2022

I believe that this doesn't make the CI state any worse, as far as I can tell. So it should be mergeable, with further CI fixes as future work.

@lf-
Copy link
Contributor Author

lf- commented Dec 4, 2022

Aah, no, the benchmark build gets silly bounds problems since cabal-syntax and cabal have to be the same version, and cabal syntax 3.8 allows building on old ghc even though this causes this problem.

@lf-
Copy link
Contributor Author

lf- commented Dec 6, 2022

I am not sure how to inject an extra constraint into the Cabal 3.6 cabal file. So I'm not sure how to fix this benchmark. Does anyone have any ideas?

@cydparser
Copy link
Contributor

@lf- Try removing Cabal from build-depends.

@lf-
Copy link
Contributor Author

lf- commented Dec 7, 2022

yep you're right @cydparser. Going to see if this fixes the older CI as well.

@michaelpj
Copy link
Collaborator

Let's see if we can get #3383 in, then?

@cydparser
Copy link
Contributor

@lf-, Cabal-syntax was removed as a dependency in the mainline branch. Try removing commit d5afada and rebasing.

@fendor fendor removed the merge me Label to trigger pull request merge label May 4, 2023
@fendor
Copy link
Collaborator

fendor commented May 4, 2023

Is this still valuable to have?
EDIT:
What I mean is, should we just rebase this PR and then it is ready to go?

@lf-
Copy link
Contributor Author

lf- commented May 4, 2023

I think yes because it would utilize nixpkgs binary caches better to not disable all test suites. I'm not sure if I'll get around to rebase super soon.

@cydparser
Copy link
Contributor

@fendor, these changes should already be in master: https://github.com/haskell/haskell-language-server/pull/3503/commits

@michaelpj
Copy link
Collaborator

I'm assuming this is stale, please reopen if not.

@michaelpj michaelpj closed this Jun 14, 2023
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.

5 participants