Skip to content

nexusmods-app: use finalAttrs#336221

Merged
corngood merged 2 commits into
NixOS:masterfrom
MattSturgeon:nexus-finalAttrs
Aug 28, 2024
Merged

nexusmods-app: use finalAttrs#336221
corngood merged 2 commits into
NixOS:masterfrom
MattSturgeon:nexus-finalAttrs

Conversation

@MattSturgeon

@MattSturgeon MattSturgeon commented Aug 21, 2024

Copy link
Copy Markdown
Contributor

Description of changes

  • nexusmods-app: use finalAttrs for recursive fixpoint
  • nexusmods-app: use buildInputs for dependencies

Follow up to #331355, make use of the finalAttrs support added to buildDotnetModule in #331398. Hopefully anything recursive in the package is now done correctly.

I was unsure how to handle the relationship between executables and meta.mainProgram, so I decided to define both explicitly for now. Perhaps buildDotnetModule should consider setting mainProgram automatically to head executables?

I also replaced the manual wrapper PATH creation with buildInputs for the desktop-file-utils dependency. AFAICT everything is still working.

Related issues:

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.11 Release Notes (or backporting 23.11 and 24.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
  • Fits CONTRIBUTING.md.

cc @corngood @l0b0


Add a 👍 reaction to pull requests you find important.

Comment thread pkgs/by-name/ne/nexusmods-app/package.nix Outdated
@MattSturgeon

Copy link
Copy Markdown
Contributor Author

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

2 packages built:
  • nexusmods-app
  • nexusmods-app-unfree

@ofborg ofborg Bot requested a review from l0b0 August 21, 2024 03:34
@ofborg ofborg Bot added 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Aug 21, 2024
@MattSturgeon MattSturgeon requested a review from corngood August 21, 2024 03:42
Comment thread pkgs/by-name/ne/nexusmods-app/package.nix Outdated
Comment thread pkgs/by-name/ne/nexusmods-app/package.nix Outdated

@eclairevoyant eclairevoyant Aug 21, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you explain the reasoning for this change? desktop-file-utils only contains binaries, so putting it in buildInputs seems like a no-op to me.

@MattSturgeon MattSturgeon Aug 21, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I guess I thought buildInputs would populate the PATH. Thanks for clarifying! I'll revert the change.

With writeShellApplication I'd use runtimeInputs, looking at the implementation, it's doing essentially what we did before:

export PATH="${lib.makeBinPath runtimeInputs}:$PATH"

Is this the best/only way to populate the PATH? Is there any idiom of making the bin-path overridable via an extra attr passed to mkDerivation (i.e. a runtimeInputs)?

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one person. label Aug 21, 2024
@MattSturgeon MattSturgeon changed the title nexusmods-app: use finalAttrs and buildInputs nexusmods-app: use finalAttrs Aug 21, 2024

@SuperSandro2000 SuperSandro2000 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Otherwise LGTM, good improvement

Comment thread pkgs/by-name/ne/nexusmods-app/package.nix Outdated
@corngood

Copy link
Copy Markdown
Contributor

@MattSturgeon Everything seems to be addressed, right? Okay if I merge?

@MattSturgeon

MattSturgeon commented Aug 28, 2024

Copy link
Copy Markdown
Contributor Author

Everything seems to be addressed, right? Okay if I merge?

@corngood Yes, everything should be good, thanks! 👍

@corngood corngood merged commit a92f0fb into NixOS:master Aug 28, 2024
@MattSturgeon MattSturgeon deleted the nexus-finalAttrs branch August 28, 2024 15:00
@Bot-wxt1221 Bot-wxt1221 mentioned this pull request Oct 22, 2024
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 12.approvals: 1 This PR was reviewed and approved by one person.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants