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

neovim: enable use of external package manager #5225

Merged
merged 2 commits into from
Apr 13, 2024

Conversation

misumisumi
Copy link
Contributor

@misumisumi misumisumi commented Apr 4, 2024

Description

#5224
Maybe it's better to leave room for custom wrapArgs (like extraArgs)

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all or nix develop --ignore-environment .#all using Flakes.

  • Test cases updated/added. See example.

  • Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

    See CONTRIBUTING for more information and recent commit messages for examples.

  • If this PR adds a new module

    • Added myself as module maintainer. See example.

Maintainer CC

@emilazy @teto

@github-actions github-actions bot added the neovim label Apr 4, 2024
@teto
Copy link
Collaborator

teto commented Apr 6, 2024

This is the kind of workaround we dont want to support in home-manager IMO, if you have to manually add dependencies then better to use the nix package directly.
The good thing is that you just need to pass arguments to the wrapper so instead you could expose wrapperArguments as a neovim module option and maintain the argument generator code in your dotfiles as a home-manager out of tree module.

@misumisumi
Copy link
Contributor Author

Yeah, this suggestion goes against nix thinking, so it seems better to provide some leeway to set args than direct support.
to correct.

@misumisumi
Copy link
Contributor Author

misumisumi commented Apr 8, 2024

I replaced to extraWrapperArgs option.
Could you please confirm?

type = with types; listOf str;
default = [ ];
example =
literalExpression ''[ "--suffix" "PATH" ":" "/path/to/bin" ]'';
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe put a working example instead, like ${golsp}/bin/golsp (make sure to escape ${golsp} suc htat it doesn't get interpolated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

also this example might not be the best because one should use extraPackages instead. Take the example from what you want to achieve with this PR, aka pass additionnal variables for the linker and make mason work.
Bonus points if you can test it but that's not mandatory, the implementation is simple and is already tested in nixpkgs.

Copy link
Contributor Author

@misumisumi misumisumi Apr 12, 2024

Choose a reason for hiding this comment

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

Take the example from what you want to achieve with this PR, aka pass additionnal variables for the linker and make mason work.

Replaced with examples and explanations of environment variable settings required for building and running binaries.

Bonus points if you can test it but that's not mandatory, the implementation is simple and is already tested in nixpkgs.

Is it okay to just add extraWrapper to no-init.nix or runtime.nix for the test?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it okay to just add extraWrapper to no-init.nix or runtime.nix for the test?

Yes, that's even better I would say. Fewer derivations to build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

add test to runtime.nix.

@misumisumi misumisumi force-pushed the feat/neovim branch 3 times, most recently from db0a8e0 to d74924b Compare April 12, 2024 15:27
@@ -417,7 +434,8 @@ in {
(neovimConfig // {
wrapperArgs = (lib.escapeShellArgs neovimConfig.wrapperArgs) + " "
+ extraMakeWrapperArgs + " " + extraMakeWrapperLuaCArgs + " "
+ extraMakeWrapperLuaArgs;
+ extraMakeWrapperLuaArgs + " "
+ concatStringsSep " " cfg.extraWrapperArgs;
Copy link
Collaborator

@teto teto Apr 12, 2024

Choose a reason for hiding this comment

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

maybe move cfg.extraWrapperARgs into the escapeShellArgs 2 line 435 which has better behavior

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 changed to add cfg.extraWrapperArgs to neovimConfig.wrapperArgs.
The example has also been modified accordingly.

"--suffix PKG_CONFIG_PATH : ${
lib.makeSearchPathOutput "dev" "lib/pkgconfig" buildDeps
}"
];
Copy link
Collaborator

Choose a reason for hiding this comment

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

you dont check this was taken into account:
in /home/teto/hm/tests/modules/programs/neovim/./runtime.nix 's nmt.script you could check if the nvim wrapper contains PKG_CONFIG_PATH, aka grep PKG_CONFIG_PATH $(which nvim) or something like that

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 added assertFileRegex to nmt.script, is this OK?

pass external arguments to neovim-unwrapper
this gives users more flexibility in managing neovim configuration
@teto teto merged commit 8fdf329 into nix-community:master Apr 13, 2024
3 checks passed
@teto
Copy link
Collaborator

teto commented Apr 13, 2024

thank you !

diniamo added a commit to diniamo/home-manager that referenced this pull request Apr 16, 2024
spotify-player: tests

chore: format

fix: declare in modules.nix

spotify-player: add news entry

spotify-player: implement review suggestions

spotify-player: fix maintainers

Squashed commit of the following:

commit 1c43dcf
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Mon Apr 15 09:42:51 2024 +0200

    ci: bump peaceiris/actions-gh-pages from 3 to 4

    Bumps [peaceiris/actions-gh-pages](https://github.com/peaceiris/actions-gh-pages) from 3 to 4.
    - [Release notes](https://github.com/peaceiris/actions-gh-pages/releases)
    - [Changelog](https://github.com/peaceiris/actions-gh-pages/blob/main/CHANGELOG.md)
    - [Commits](peaceiris/actions-gh-pages@v3...v4)

    ---
    updated-dependencies:
    - dependency-name: peaceiris/actions-gh-pages
      dependency-type: direct:production
      update-type: version-update:semver-major
    ...

    Signed-off-by: dependabot[bot] <support@github.com>
    Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

commit 59d50bc
Author: Nathan Henrie <n8henrie@users.noreply.github.com>
Date:   Mon Apr 15 01:40:27 2024 -0600

    espanso: enable module on darwin

commit 9f32c66
Author: Jose Plana <Jose.Plana.Mario@telefonica.com>
Date:   Wed Apr 10 13:34:46 2024 +0200

    k9s: configuration files in Darwin without XDG

    Support alternate configuration files for k9s in darwin where XDG is
    not mandated and k9s expects configuration files in
    `~/Library/Application Support/k9s/`.

commit 76a1650
Author: Jose Plana <Jose.Plana.Mario@telefonica.com>
Date:   Wed Apr 10 10:24:46 2024 +0200

    k9s: fix typos in configuration file names

commit 630a099
Author: Philipp Mildenberger <philipp@mildenberger.me>
Date:   Sun Apr 14 08:58:16 2024 +0200

    nushell: fix nushell config path on darwin

commit 4cc3c91
Author: home-manager-bot <106474382+home-manager-bot@users.noreply.github.com>
Date:   Sun Apr 14 08:56:05 2024 +0200

    flake.lock: Update

    Flake lock file updates:

    • Updated input 'nixpkgs':
        'github:NixOS/nixpkgs/fd281bd6b7d3e32ddfa399853946f782553163b5' (2024-04-03)
      → 'github:NixOS/nixpkgs/1042fd8b148a9105f3c0aca3a6177fd1d9360ba5' (2024-04-10)

    Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

commit f33d508
Author: Mitchell Skaggs <skaggsm333@gmail.com>
Date:   Sat Apr 13 12:59:33 2024 -0500

    rio: remove redundant `lib.mdDoc` call

    This is an error as of NixOS/nixpkgs#303841

    It seems to have been missed in nix-community#4215

commit 8fdf329
Author: MiSumiSumi <dragon511southern@gmail.com>
Date:   Sat Apr 13 23:50:15 2024 +0900

    neovim: enable use of external package manager (nix-community#5225)

    * neovim: add extraWrapperArgs option

    pass external arguments to neovim-unwrapper
    this gives users more flexibility in managing neovim configuration

    * neovim: add test for `extraWrapperArgs`

commit 40ab43a
Author: Bryn Edwards <bryn@converge-digital.com>
Date:   Sat Apr 13 07:27:43 2024 +0100

    foot: set PATH in server's systemd unit file

    If not set, foot's terminal spawning shortcut will not work as the
    `footclient` binary is not on the server's PATH.

commit 3135748
Author: Ramses <ramses@well-founded.dev>
Date:   Wed Apr 10 16:39:52 2024 +0200

    fish: use the subcommand style for the status command (nix-community#4584)

    The flag style has been deprecated and will eventually be removed.

commit 18f89ef
Author: Tony Zorman <soliditsallgood@mailbox.org>
Date:   Wed Apr 10 08:29:32 2024 +0200

    firefox: add containersForce flag

    Firefox, upon exit, creates the default containers.json file in place of
    the one that home-manager created. This leads to errors when switching
    to a new profile, as home-manager is careful with overwriting existing
    files. The added option toggles that behaviour.

    Closes: nix-community#4989

commit b00d0e4
Author: Jack W <29169102+Jack5079@users.noreply.github.com>
Date:   Tue Apr 9 14:48:15 2024 -0400

    bun: add module

commit 40a9961
Author: Mario Rodas <marsam@users.noreply.github.com>
Date:   Tue Apr 9 01:57:29 2024 -0500

    fzf: add compatibility with fzf≥0.48.0

    fzf 0.48.0 [1] changed the way it integrates with shells.

    [1] https://github.com/junegunn/fzf/releases/tag/0.48.0

commit a561ad6
Author: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Date:   Sun Apr 7 03:59:32 2024 +0000

    flake.lock: Update

    Flake lock file updates:

    • Updated input 'nixpkgs':
        'github:NixOS/nixpkgs/d8fe5e6c92d0d190646fb9f1056741a229980089' (2024-03-29)
      → 'github:NixOS/nixpkgs/fd281bd6b7d3e32ddfa399853946f782553163b5' (2024-04-03)

commit b787726
Author: Smaug123 <patrick+github@patrickstevens.co.uk>
Date:   Fri Apr 5 14:05:00 2024 +0100

    home-manager: extract inline shell script to file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants