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

Fish completions fail for missing attribute 'name' #2813

Closed
stopbystudent opened this issue Mar 21, 2022 · 7 comments · Fixed by #4492
Closed

Fish completions fail for missing attribute 'name' #2813

stopbystudent opened this issue Mar 21, 2022 · 7 comments · Fixed by #4492

Comments

@stopbystudent
Copy link

stopbystudent commented Mar 21, 2022

After a recent update of my nixpkgs (unstable channel), home-manager will not rebuild anymore if I enable fish. Error message as follows:

error: attribute 'name' missing

       at /nix/store/wfgrdl7b24fxwb2rqw7nz1x8wd5fk3ch-source/modules/programs/fish.nix:308:30:

          307|         generateCompletions = package:
          308|           pkgs.runCommand "${package.name}-fish-completions" {
             |                              ^
          309|             src = package;

My suspicion is that the code at

generateCompletions = package:
pkgs.runCommand "${package.name}-fish-completions" {
src = package;
nativeBuildInputs = [ pkgs.python2 ];
buildInputs = [ cfg.package ];
preferLocalBuild = true;
} ''

relies too much on the name attribute which often is replaced by pname and version, as seems to be the case in nixpkgs' definition for fish. I have seen that the stdenv checker does use a getName function to retrieve the package name (see here) but that might be overkill since seemingly only the module here is affected.

I'm not entirely sure (and can't test at the moment) but maybe using pkgs.runCommand "${package.pname}-${package.version}-fish-completions" instead of the current package.name would already be enough?

Edit: Actually, it seems that not all packages have pname either, so I just tried running with the following snippet

        generateCompletions = let
          getName = attrs: attrs.name or ("${attrs.pname or "«name-missing»"}-${attrs.version or "«version-missing»"}");
        in package:
          pkgs.runCommand "${getName package}-fish-completions" {
            src = package;
            nativeBuildInputs = [ pkgs.python2 ];
            buildInputs = [ cfg.package ];
            preferLocalBuild = true;
          } ''
            mkdir -p $out
            if [ -d $src/share/man ]; then
              find $src/share/man -type f \
                | xargs python ${cfg.package}/share/fish/tools/create_manpage_completions.py --directory $out \
                > /dev/null
            fi
          '';

and it at least solved my issue. I don't know whether this is a good place to insert such a getName function (taken from the source linked above) or whether it should be imported somehow, hence I won't prepare a PR.

@berbiche
Copy link
Member

Hi,

The assumption in Home Manager's logic is that the name attribute is always set on a derivation.

Using the primitive builtins.derivation { } gives an error if the name attribute is not specified.

I don't think there's anything to fix on our side though I might be wrong. If you think so, a PR is welcome!

@stopbystudent
Copy link
Author

As I have described in the issue, this assumption does not hold. Sometimes there's only pname and version, sometimes only name. The function from the issue description solved my issue after a home-manager update broke my config. If you don't want to include it, that's fine, I'll keep it in my personal fork.

I would prepare a PR if I knew this affected only the fish completions. But if this is a general assumption that does not hold which spreads throughout the codebase I'm not sure a PR to change it at this place would justify the inconsistencies. So I guess, I'll leave it at this status quo.

@rycee
Copy link
Member

rycee commented Nov 10, 2022

@xervon
Copy link

xervon commented Nov 11, 2022

I just noticed the following issue in nixpkgs: NixOS/nixpkgs#103997
It seems like the plan for the future is for packages to have pname+version instead of name so using either getName or at least falling back to pname+version seems reasonable.

@stopbystudent
Copy link
Author

I just noticed the following issue in nixpkgs: NixOS/nixpkgs#103997 It seems like the plan for the future is for packages to have pname+version instead of name so using either getName or at least falling back to pname+version seems reasonable.

Thank you for linking that. I just noticed I did not link it in the original issue because I thought it was a well-known plan (after all, it has been communicated for nearly two years now).

This long-term effort is also the reason why I mentioned that berbiche's statement

The assumption in Home Manager's logic is that the name attribute is always set on a derivation.

obviously shows that home-manager is not consistently not prepared for this effort, so I should repeat my assessment that fixing it in the fish completions is the wrong place because it would lead to inconsistencies and only partial improvements. Therefore, I guess this should be a properly worded issue on its own if the home-manager team would like to remove the assumption from the code.

General disclaimer: I haven't even checked whether the getName function is still required for the fish completions, but I use it in my personal fork of home-manager and it still works.

@rycee
Copy link
Member

rycee commented Nov 11, 2022

I think it wouldn't be controversial to create a PR that changes the module to use Nixpkgs' getName so you should absolutely feel free to try it out and open a PR if it work well for you.

I'll reopen this issue for now.

@stale
Copy link

stale bot commented Feb 10, 2023

Thank you for your contribution! I marked this issue as stale due to inactivity. Please be considerate of people watching this issue and receiving notifications before commenting 'I have this issue too'. We welcome additional information that will help resolve this issue. Please read the relevant sections below before commenting.

If you are the original author of the issue

  • If this is resolved, please consider closing it so that the maintainers know not to focus on this.
  • If this might still be an issue, but you are not interested in promoting its resolution, please consider closing it while encouraging others to take over and reopen an issue if they care enough.
  • If you know how to solve the issue, please consider submitting a Pull Request that addresses this issue.

If you are not the original author of the issue

  • If you are also experiencing this issue, please add details of your situation to help with the debugging process.
  • If you know how to solve the issue, please consider submitting a Pull Request that addresses this issue.

Memorandum on closing issues

Don't be afraid to manually close an issue, even if it holds valuable information. Closed issues stay in the system for people to search, read, cross-reference, or even reopen – nothing is lost! Closing obsolete issues is an important way to help maintainers focus their time and effort.

@stale stale bot added the status: stale label Feb 10, 2023
a-kenji added a commit to a-kenji/home-manager that referenced this issue Sep 22, 2023
Query the `pname` and `version` attributes for completion generation,
if the `name` attribute is not available.

Fixes nix-community#2813
a-kenji added a commit to a-kenji/home-manager that referenced this issue Sep 22, 2023
Query the `pname` and `version` attributes for completion generation,
if the `name` attribute is not available.

Fixes nix-community#2813
rycee pushed a commit to a-kenji/home-manager that referenced this issue Oct 6, 2023
Query the `pname` and `version` attributes for completion generation,
if the `name` attribute is not available.

Fixes nix-community#2813
@rycee rycee closed this as completed in 31a27e4 Oct 6, 2023
fufexan pushed a commit to fufexan/home-manager that referenced this issue Feb 24, 2024
Query the `pname` and `version` attributes for completion generation,
if the `name` attribute is not available.

Fixes nix-community#2813
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants