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

haskell.packages.ghc910.haskell-language-server: fix package #337720

Merged
merged 1 commit into from
Aug 29, 2024

Conversation

eldritch-cookie
Copy link
Contributor

Description of changes

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

Copy link
Member

@maralorn maralorn left a comment

Choose a reason for hiding this comment

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

LGTM

One optional remark

@@ -120,4 +123,8 @@ self: super: {
lukko_0_1_2 = dontCheck super.lukko_0_1_2; # doesn't compile with tasty ==1.4.*
resolv = dontCheck super.resolv; # doesn't compile with filepath ==1.5.*
primitive-unlifted = dontCheck super.primitive-unlifted; # doesn't compile with primitive ==0.9.*

haskell-language-server = disableCabalFlag "retrie" (disableCabalFlag "hlint" (disableCabalFlag "stylishhaskel" (super.haskell-language-server.override {stylish-haskell = null;retrie = null;apply-refact=null;hlint = null;})));
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to convert this to use lib.pipe as discussed on matrix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think 3 is about the limit where doing directly still is acceptable.

Copy link
Member

Choose a reason for hiding this comment

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

Well, I personally use lib.pipe as soon as I hit 2 functions, but sure.

@maralorn
Copy link
Member

Ah, wait one more thing we should enable the hls job for 9.10 now. Can you drop the right line from release-haskell.nix (search for haskell-language-server)?

Copy link
Contributor

@eclairevoyant eclairevoyant left a comment

Choose a reason for hiding this comment

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

breaks eval, related to #336141

@eldritch-cookie
Copy link
Contributor Author

eldritch-cookie commented Aug 27, 2024

breaks eval, related to #336141

i am so confused, how is this in anyway caused by my PR?
how do if fix this?

@@ -226391,7 +226411,6 @@ self: {
];
description = "Haskell binding to OpenCV-3.x extra modules";
license = lib.licenses.bsd3;
hydraPlatforms = lib.platforms.none;
Copy link
Contributor

@eclairevoyant eclairevoyant Aug 27, 2024

Choose a reason for hiding this comment

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

It's due to the removal of this line, which in turn eval'd opencv, which in turn resulted in the eval failure since opencv3 no longer exists.
Technically it was caused by 336141 not removing the opencv3 reference properly, but I'm not sure why we're touching opencv here when this PR is about hls.

Copy link
Contributor

Choose a reason for hiding this comment

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

Tagging @emilazy as you might have more insight into this, I saw your comment about the dangling reference being desirable, and I don't get it.

Copy link
Member

Choose a reason for hiding this comment

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

So, this file is generated by cabal2nix and any manual modifications to it will be blown away. I talked about it with the Haskell maintainers and they told me that the dangling reference is fine because it’s guarded by callPackage and the package is already marked as broken, and that it’s declaring something “true” about the package even if the thing it depends on is no longer, uh, there.

Getting rid of the dangling reference would require getting a PR into cabal2nix and it bumped as part of a haskell-updates cycle, which didn’t seem worth it to me for a broken package, so I didn’t. It would also mean that cabal2nix would handle it incorrectly on 24.05, where I think this package still actually works, I guess, but I don’t know if you’re expected to be able to use a new cabal2nix with an older Nixpkgs like that.

Anyway it seems wrong to touch this line and probably that should be reverted? Any manual modifications will get blown away by the next cabal2nix run, though.

I’m not really an expert in the Haskell machinery, I just had to touch it to get rid of dependencies. cc @maralorn

Copy link
Member

Choose a reason for hiding this comment

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

Okay so I guess the problem is that opencv-extra got removed from the transitive-broken list in this PR, presumably because of an automatic cabal2nix thing, presumably because of something to do with me removing the overrides in #336110 or opencv3 getting removed. Beyond that I don’t know why it would happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i didn't manually touch hackage-packages.nix i just modified the configuration file of the generator and regenerated, crucially i didn't touch broken.yaml. the problem is that opencv-extra wasn't there but only on the transitive broken file which is regenerated when you run the script.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the regenerate-hackage-packages script has this in the documentation

Unless --fast is used, it will then use the generated nix expression by
running regenerate-transitive-broken-packages.sh which updates the transitive-broken.yaml
file. Then it re-runs hackage2nix.

so exactly that, i have no idea why opencv-extra does not depend on any of the other opencv packages

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure I recognise that it's not your fault, rather you happened to be the first person touching haskell packages since the other PR. Still I don't think merging a PR that will break the base branch is the way to go here.

Copy link
Member

Choose a reason for hiding this comment

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

So, opencv-extra does list opencv as a dependency in hackage-packages.nix. Is it… picking up the native library, instead of the Haskell package that it depends on? And somehow that’s different now to before my PRs and makes it no longer seem transitively broken to the machinery?

Copy link
Member

Choose a reason for hiding this comment

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

Huh, I can look into this later today and fix it on haskell-updates. Then you can rebase and don't need to worry about this.

Copy link
Member

@maralorn maralorn Aug 28, 2024

Choose a reason for hiding this comment

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

Done, I hope.

@eldritch-cookie
Copy link
Contributor Author

rebased, and everything seems fine

@eclairevoyant eclairevoyant dismissed their stale review August 29, 2024 14:12

changes addressed

@maralorn maralorn merged commit e87381d into NixOS:haskell-updates Aug 29, 2024
9 of 11 checks passed
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Aug 29, 2024
jmatsushita added a commit to jmatsushita/nixpkgs that referenced this pull request Oct 2, 2024
jmatsushita added a commit to jmatsushita/nixpkgs that referenced this pull request Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: haskell 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants