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

Add exe attribute to packages #158461

Closed
wants to merge 7 commits into from
Closed

Add exe attribute to packages #158461

wants to merge 7 commits into from

Conversation

roberth
Copy link
Member

@roberth roberth commented Feb 7, 2022

Motivation for this change

Adds an exe attribute as a shorthand for the main executable of a package.

Closes #138418

Things done
  • Built on platform(s)
    • evaluates to the same, so no build required
    • 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.05 Release Notes (or backporting 21.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS and removed 6.topic: stdenv Standard environment labels Feb 7, 2022
@roberth roberth mentioned this pull request Feb 7, 2022
6 tasks
@roberth roberth changed the title Add exe Add exe attribute to packages Feb 7, 2022
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Feb 7, 2022
@Synthetica9
Copy link
Member

Synthetica9 commented Feb 7, 2022

I always interpreted meta.mainProgram as "mainprogram is different from pname, please use this instead". This would suggest a package like hello should still set meta.mainProgram = "hello", is that correct?

@roberth
Copy link
Member Author

roberth commented Feb 7, 2022

I always interpreted meta.mainProgram as "mainprogram is different from pname, please use this instead". This would suggest a package like hello should still set meta.mainProgram = hello, is that correct?

When mainProgram is set, we also know that there is a prominent single program, which is not something you can infer from pname. Hence, setting mainProgram improves our data quality, so that's a good thing.

I should also note that exe isn't mandatory, thanks to laziness.

@Synthetica9
Copy link
Member

When mainProgram is set, we also know that there is a prominent single program, which is not something you can infer from pname. Hence, setting mainProgram improves our data quality, so that's a good thing.

Wouldn't there be a lot of duplication for not a lot of gain? Shouldn't we set mainProgram = null if there is no clear main program, but default it to pname otherwise? I think requiring 95% of packages to set this for marginal gain is not the way to go tbh

@edolstra
Copy link
Member

edolstra commented Feb 7, 2022

It seems better to me to have a function that returns the main program for a package, rather than pollute the attribute namespace of every package with a magic attribute. I.e. I'd prefer getMainProgram pkgs.hello to pkgs.hello.exe.

@roberth
Copy link
Member Author

roberth commented Feb 7, 2022

rather than pollute the attribute namespace of every package with a magic attribute.

I am well aware and I agree that we shouldn't add attributes lightly, though we have to consider:
Is it pollution or convenience for a very common use case?
Is it magic or merely a derived attribute similar to the outputs and drvPath?

In my view, the brevity is worth it.
That's a rare statement from someone who prefers previous over prev.

@edolstra
Copy link
Member

edolstra commented Feb 7, 2022

It's not so much about the brevity of exe versus getMainProgram, but about the namespace of derivation attributes. Adding derived attributes like exe means that packages can't have their own attributes that happen to be named exe. It also adds an extra thunk to every package, which does eventually add up if we were to do every derived package value in the same style.

Derivations that define a regular `exe` attribute but not a
`meta.mainProgram` will behave exactly the same again.
@roberth
Copy link
Member Author

roberth commented Feb 8, 2022

Adding derived attributes like exe means that packages can't have their own attributes that happen to be named exe.

That wasn't quite the case. Defining the attr worked just fine and I've updated the solution so it can be read back as an attribute as well.
A change to user code is only necessary if their derivation defines both exe and meta.mainProgram and it needs to read back the exe attr not just in the builder but in Nix code.

It also adds an extra thunk to every package, which does eventually add up if we were to do every derived package value in the same style.

Allocations are up by 0.45%. Final heap size is up by a slightly higher amount of approximately 0.7% (it gets rounded). CPU time is too noisy in the evaluation report. It shows an "improvement" in the previous report, but total allocations are a more usable proxy for time, so probably also near 0.45%. See https://github.com/NixOS/nixpkgs/pull/158461/checks?check_run_id=5092093463 or latest report in the commit statuses.

which does eventually add up if we were to do every derived package value in the same style.

I agree that we should only do it for very few; certainly not every derived value.

@Synthetica9

Wouldn't there be a lot of duplication for not a lot of gain?

In absolute terms, yes there would, but it is not a maintenance hazard.
Bad data is harder to recover from than missing data.
This would be somewhat of a tangential discussion though, that we don't have to resolve in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.kind: enhancement Add something new 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: user experience 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants