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

kakoune-lsp: 16.0.0 -> 17.1.1 #302761

Merged
merged 4 commits into from
Aug 12, 2024
Merged

Conversation

deviant
Copy link
Member

@deviant deviant commented Apr 9, 2024

Description of changes

The Perl patch no longer applied, so along with creating a new version of it, I took the liberty to clean up the package overall.

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.05 Release Notes (or backporting 23.05 and 23.11 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.

@matthiasbeyer
Copy link
Contributor

Result of nixpkgs-review pr 302761 run on x86_64-linux 1

1 package built:
  • kakoune-lsp (kakounePlugins.kakoune-lsp)

Copy link
Member

@alexshpilkin alexshpilkin left a comment

Choose a reason for hiding this comment

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

Verified basic functionality (clangd integration).

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Apr 23, 2024
@2xsaiko
Copy link
Contributor

2xsaiko commented May 1, 2024

This (and also the current version of this package) needs a newer Kakoune version, it broke my configuration:

Error: 2:9: 'add-highlighter': wrong parameter count (NOTE: lsp-inlay-diagnostics-enable requires Kakoune >= 2024)

@alexshpilkin
Copy link
Member

@2xsaiko Right, inlay stuff in kakoune-lsp ≥ 16 requires git (unreleased) Kakoune; but at least ≥ 17 (this PR, or anything including kakoune-lsp/kakoune-lsp@86ceae0) will tell you what’s wrong instead of crashing incomprehensibly.

As far as cutting a release of Kakoune, we’re probably at the mercy of @mawww here as opposed to the Nixpkgs maintainerdom—you don’t usually get unreleased versions in Nixpkgs unless there’s no choice.

@deviant
Copy link
Member Author

deviant commented May 1, 2024

To anyone who wants to use inlay diagnostics with the current stable version of Kakoune, add the following to the patches of kakoune-unwrapped:

# "Support -after switch for flag-lines highlighter"
# Needed by kakoune-lsp v16.0.0+ for inlay diagnostics.
(fetchpatch2 {
  url = "https://github.com/mawww/kakoune/commit/c124c8f517c921bee2017cdf5081f3e8b98e27b9.patch";
  hash = "sha256-R+B29aKb9WGgEmNmVN0ZL0wPWrBFcTYglVBfhCjKENQ=";
  includes = [ "src/highlighters.cc" ];
})

I've been using this for a while with no issues.

Copy link
Contributor

@alois31 alois31 left a comment

Choose a reason for hiding this comment

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

The new version seems to work very well, including code actions when perl is not in PATH (which was previously broken).

@wegank wegank added 12.approvals: 2 This PR was reviewed and approved by two reputable people and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels May 4, 2024
@L-as
Copy link
Member

L-as commented Jul 18, 2024

what's blocking this from being merged?

@QiBaobin
Copy link

QiBaobin commented Jul 28, 2024

can we review and merge this? There is even an 17.1.1 out for a while.

@alyssais alyssais added the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 9, 2024
@deviant deviant removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 11, 2024
@deviant
Copy link
Member Author

deviant commented Aug 11, 2024

I've fixed the conflict. Can someone merge this finally? Once that's done it'd be a simple matter for someone to bump it from there to the latest version.

@deviant deviant marked this pull request as draft August 11, 2024 08:56
@deviant deviant force-pushed the kakoune-lsp-17.0.1 branch from bcf9c0c to 8d9ed73 Compare August 11, 2024 09:08
@deviant deviant marked this pull request as ready for review August 11, 2024 09:08
@deviant
Copy link
Member Author

deviant commented Aug 11, 2024

Correction: have now fixed the conflict without using the web UI (which somehow produced an incorrect commit). Should be good to go now.

@ofborg ofborg bot requested a review from philiptaron August 11, 2024 09:20
@philiptaron philiptaron mentioned this pull request Aug 12, 2024
13 tasks
pkgs/by-name/ka/kakoune-lsp/Hardcode-perl.patch Outdated Show resolved Hide resolved
pkgs/by-name/ka/kakoune-lsp/Hardcode-perl.patch Outdated Show resolved Hide resolved
pkgs/by-name/ka/kakoune-lsp/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ka/kakoune-lsp/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ka/kakoune-lsp/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ka/kakoune-lsp/package.nix Outdated Show resolved Hide resolved
@philiptaron
Copy link
Contributor

Thanks for keeping this patch running. I'm back in the saddle and looking to merge once a few things are tidied up.

@deviant
Copy link
Member Author

deviant commented Aug 12, 2024

I'm not looking to make further changes on this, sorry. In the several months it took for anyone to actually look at this PR, I've entirely stopped using Nix on any of my machines, so this isn't really a priority for me now.

@philiptaron
Copy link
Contributor

I'm not looking to make further changes on this, sorry.

Understood. Would you mind if I did so?

@deviant
Copy link
Member Author

deviant commented Aug 12, 2024

You're welcome to! :) Please try to keep my authorship metadata intact, depending on how you go about this.

@philiptaron
Copy link
Contributor

You're welcome to! :) Please try to keep my authorship metadata intact, depending on how you go about this.

I'll do my very best. 🙇🏻

@philiptaron philiptaron changed the title kakoune-lsp: 16.0.0 -> 17.0.1 kakoune-lsp: 16.0.0 -> 17.1.1 Aug 12, 2024
@philiptaron philiptaron merged commit c625734 into NixOS:master Aug 12, 2024
9 of 12 checks passed
@L-as
Copy link
Member

L-as commented Aug 12, 2024

Thanks!

@deviant deviant deleted the kakoune-lsp-17.0.1 branch August 13, 2024 07:30
@L-as
Copy link
Member

L-as commented Aug 16, 2024

Funnily enough this broke basically immediately because of #332957

@philiptaron
Copy link
Contributor

I can confirm the following update now makes it build.

@philiptaron
Copy link
Contributor

Funnily enough this broke basically immediately because of #332957

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 12.approvals: 2 This PR was reviewed and approved by two reputable people
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants