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.

Sign up for GitHub

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

Jump to bottom

treewide: fix lints Arg to lib.optional is a list *Flags not a list #337743

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

treewide: fix lints Arg to lib.optional is a list *Flags not a list #337743

wants to merge 1 commit into from
+66 −66

Conversation

Artturin
Copy link
Member

https://www.github.com/nix-community/nixpkgs-lint

Description of changes

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.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: python 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: qt/kde 8.has: module (update) This PR changes an existing module in `nixos/` 6.topic: rust 6.topic: systemd 6.topic: php labels Aug 27, 2024
@Artturin Artturin marked this pull request as ready for review August 28, 2024 14:44
@K900
Copy link
Contributor

K900 commented Aug 28, 2024

Why are we not rebuild 0 here?

@flokli
Copy link
Contributor

flokli commented Aug 28, 2024

This is missing some explanation - shouldn't these which add a list with a single element not keep lib.optional, but remove the surrounding list?

@drupol
Copy link
Contributor

drupol commented Aug 28, 2024

This is missing some explanation - shouldn't these which add a list with a single element not keep lib.optional, but remove the surrounding list?

I would keep lib.optionals by default because it requires the brackets. This is easier for everybody (especially newcomers) to understand what's the returned value.

@Artturin
Copy link
Member Author

Why are we not rebuild 0 here?

I'll check later, at the nixos helsinki meet up rn.

@SuperSandro2000
Copy link
Member

Why are we not rebuild 0 here?

probably because some nix variables where turned into bash strings and the type of those changed?

This is missing some explanation - shouldn't these which add a list with a single element not keep lib.optional, but remove the surrounding list?

IMO doesn't really matter in the end. Removing the [] could cause a bigger diff because they are sometimes not on one line.

PS: Before anyone gets the idea to make a treewide PR to convert lib.optional to lib.optionals [ ]. Just don't there is no benefit and just noise which will crawl into many open PRs.

SuperSandro2000
SuperSandro2000 reviewed Aug 28, 2024
View reviewed changes
pkgs/by-name/xr/xr-hardware/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/sm/smuxi/package.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/bokeh/default.nix Outdated Show resolved Hide resolved
pkgs/tools/networking/tcpdump/default.nix

nativeCheckInputs = [ perl ];

buildInputs = [ libpcap ];

configureFlags = lib.optional (stdenv.hostPlatform != stdenv.buildPlatform) "ac_cv_linux_vers=2";
configureFlags = lib.optionals (stdenv.hostPlatform != stdenv.buildPlatform) [ "ac_cv_linux_vers=2" ];
Copy link
Member

Choose a reason for hiding this comment

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

This was also correct before I think

Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing, it feels more correct to use lib.optionals here. IMHO, it is important for contributors to explicitly see the brackets to understand that this is a list.

Copy link
Member Author

Choose a reason for hiding this comment

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

*Flags not a list applies

ElvishJerricco
ElvishJerricco requested changes Aug 28, 2024
View reviewed changes
Copy link
Contributor

@ElvishJerricco ElvishJerricco left a comment

Choose a reason for hiding this comment

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

Well the darwin rebuild count is over 5000 so either this needs to target staging or this needs to be fixed to rebuild 0. I'd prefer the latter.

@Artturin
Copy link
Member Author

Artturin commented Aug 29, 2024 •
edited
Loading

The rebuilds are due to fixing Arg to lib.optional is a list in the dependency attributes

image

The dependency choosing in mkDerivation does not work with nested lists

nixpkgs/pkgs/stdenv/generic/make-derivation.nix

Line 304 in 5402c1e

dependencies = [

I'll split the libass change out which should reduce the rebuild count drastically but not to 0.

@K900
Copy link
Contributor

K900 commented Aug 29, 2024

Fun. Maybe split out all the rebuild-0 bits and then just send the rest to staging?

@Artturin Artturin marked this pull request as draft August 29, 2024 22:48
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@drupol drupol drupol approved these changes

@ElvishJerricco ElvishJerricco ElvishJerricco requested changes

@jtojnar jtojnar Awaiting requested review from jtojnar

@aanderse aanderse Awaiting requested review from aanderse

@globin globin Awaiting requested review from globin

@Ma27 Ma27 Awaiting requested review from Ma27

@talyz talyz Awaiting requested review from talyz

@RaitoBezarius RaitoBezarius Awaiting requested review from RaitoBezarius

@K900 K900 Awaiting requested review from K900

@NickCao NickCao Awaiting requested review from NickCao

@ttuegel ttuegel Awaiting requested review from ttuegel

@Mic92 Mic92 Awaiting requested review from Mic92

@zowoq zowoq Awaiting requested review from zowoq

@winterqt winterqt Awaiting requested review from winterqt

@figsoda figsoda Awaiting requested review from figsoda

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: php 6.topic: python 6.topic: qt/kde 6.topic: rust 6.topic: systemd 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 501+ 10.rebuild-darwin: 1001-2500 10.rebuild-linux: 11-100
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Artturin @K900 @flokli @drupol @SuperSandro2000 @ElvishJerricco @wegank