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

perlPackages: add default meta.mainProgram #176398

Merged
merged 1 commit into from
Jun 6, 2022

Conversation

zimbatm
Copy link
Member

@zimbatm zimbatm commented Jun 5, 2022

Description of changes

Because perl packages are prefixed with the perl version, it means that
the lib.getExe heuristic will never point to the binary name. So we
provide the meta.mainProgram that overrides that, using the original
pname or parsed name. It's not perfect but should yield better results
already.

If you're looking for an example: nix run .#pgbadger used to be broken

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 22.11 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

Because perl packages are prefixed with the perl version, it means that
the `lib.getExe` heuristic will never point to the binary name. So we
provide the meta.mainProgram that overrides that, using the original
pname or parsed name. It's not perfect but should yield better results
already.
@zakame
Copy link
Member

zakame commented Jun 6, 2022

Maybe you could get in touch with @malob as he has #172413 also open that's adding this.

@zimbatm
Copy link
Member Author

zimbatm commented Jun 6, 2022

Thanks for letting me know. I removed the individual package fixes to keep this PR focused on the default mainProgram change. I will merge this so the other PR can be rebased on top, I believe he will be able to remove a bunch of overrides.

@zimbatm zimbatm merged commit ff7b216 into NixOS:master Jun 6, 2022
@zimbatm zimbatm deleted the perl-mainProgram branch June 6, 2022 12:35
@malob
Copy link
Member

malob commented Jun 6, 2022

This will definitely get nix run working by default for a bunch of packages, but I want to check whether this is the right approach.

This change is explicitly setting the mainProgram to the incorrect value for many packages, so that others have the right value. I definitely get a little bit of a code smell from this, in that it seems like needing to do this is a symptom of a deeper underlying problem.

I don't have any real experience with perlPackages, so I could be missing important consideration, but if I was trying to implement a general fix for this issue, I'd first explore making a change to how Perl packages are packaged so that the pname reflects the name of the package (rather then being prefixed with the Perl version). This is how pythonPackages does things, pname is just the name of the package, and name is the packages's name prefixed with the Python version and suffixed with the package's version.

@zimbatm
Copy link
Member Author

zimbatm commented Jun 7, 2022

Sounds good, if you do that change then mine can be reverted. I'm not a perl maintainer so I was only looking for a minimal change.

@malob
Copy link
Member

malob commented Jun 7, 2022

#176792

SuperSandro2000 pushed a commit that referenced this pull request Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants