kana: init at 1.4 #292120

Aleksanaa · 2024-02-28T17:17:40Z

Description of changes

just for fun

./result/
├── bin
│   └── kana
└── share
    ├── applications
    │   └── com.felipekinoshita.Kana.desktop
    ├── glib-2.0
    ├── gsettings-schemas
    │   └── kana-1.4
    │       └── glib-2.0
    │           └── schemas
    │               ├── com.felipekinoshita.Kana.gschema.xml
    │               └── gschemas.compiled
    ├── icons
    │   └── hicolor
    │       ├── scalable
    │       │   └── apps
    │       │       └── com.felipekinoshita.Kana.svg
    │       └── symbolic
    │           └── apps
    │               └── com.felipekinoshita.Kana-symbolic.svg
    ├── kana
    │   └── resources.gresource
    └── metainfo
        └── com.felipekinoshita.Kana.metainfo.xml

Things done

Add a 👍 reaction to pull requests you find important.

pkgs/by-name/ka/kana/package.nix

AndersonTorres · 2024-02-28T18:04:42Z

pkgs/by-name/ka/kana/package.nix

+    domain = "gitlab.gnome.org";
+    owner = "fkinoshita";
+    repo = "Kana";
+    rev = "v${version}";


Suggested change

rev = "v${version}";

rev = "v${finalAttrs.version}";

what is the actual advantage of this? it's not like overriding version will work properly, since all the fetchers are fixed-output

in fact, this could even cause a derivation to build fine on one system, but fail on another (i've had this happen when updating packages and forgetting to invalidate the source hash, this could be extremely confusing to less technical users who haven't touched the inside of nixpkgs)

what is the actual advantage of this?

Get rid of rec.

but the only problem with rec that page mentions is the possibility for infinite recursion, which is equally possible with the finalAttrs pattern.

i asked what the advantage of getting rid of rec is, and you gave me quite a recursive answer.

finalAttrs is preferable in cases where attributes of the derivation are actually used within the derivation - here the only places where it's used are in src and cargoDeps, which doesn't really save much effort when someone goes to override.

And this specific usage is at least somewhat contentious: see #293452 (comment)

I do not consider this worth blocking a review on.

pkgs/by-name/ka/kana/package.nix

AndersonTorres · 2024-02-28T18:05:24Z

pkgs/by-name/ka/kana/package.nix

+    gst-plugins-good
+  ]);
+
+  meta = with lib; {


Suggested change

meta = with lib; {

meta = {

Nested with is bad.

I know nested with is bad, but the only thing that is nested here is the name of the maintainer, while writing "lib" everywhere is very ugly. And this is almost a convention in nixpkgs.

And this is almost a convention in nixpkgs.

You was complaining about finalAttrs not being written convention or enforced guideline, but now you are arguing about with lib; being a consuetudinary convention and enforcing it? Please be consistent.

Tracking issue: remove overuses of with lib; #208242

You was complaining about finalAttrs not being written convention or enforced guideline, but now you are arguing about with lib; being a consuetudinary convention and enforcing it? Please be consistent.

No, my request was consistent actually: I'm requesting a formal discussion about whether a particular format or feature should be adopted, and a documentation change indicating the best practice to follow (I know there is one waiting to be merged now). (And, if possible, automatically suggested changes by CI.) Before that, I'm always going to follow the common practice in Nixpkgs, which I've always found clean and easy, but I'm not going to debate if that's the best practice.

I have long found that following some suggestions of newer modifications can be a problem: for example, when adding environment variables, although some committers think that we should always prefer env.XXX, there are also commiters suggesting me to remove the leading env. This concern also affects my opinion of this suggested change, because I have added more than 30 packages in the past year, but this is the first time I have received this request, and after receiving this request, my other PRs submitted at the same time still passed the review. So I doubt the progress of committers' discussions on these issues.

Besides that, yes, the changes are good to go and applying them doesn't seem to introduce new problems for me.

No, my request was consistent actually: I'm requesting a formal discussion about whether a particular format or feature should be adopted, and a documentation change indicating the best practice to follow (I know there is one waiting to be merged now).

Since nix.dev is NixOS-blessed documentation, their best pratices advices are authoritative, even if not vinculative.
Given the above,

finalAttrs gets rid of rec, and getting rid of rec is one of best practices

indeed finalAttrs is documented in the NixOS stdenv docs, have you read about it?

no nested with aligns with avoiding with

I have long found that following some suggestions of newer modifications can be a problem: for example, when adding environment variables, although some committers think that we should always prefer env.XXX, there are also commiters suggesting me to remove the leading env.

Currently, attributes not recognized by (say) stdenv.mkDerivation are treated as environment vars to the shell.

It can bring problematic issues, like "oh, i wrote propBuildInputs instead of propagatedBuildInputs, and this bug goes undetected".

By using env you are explicitly saying this is an environment variable.

Indeed this is one of examples given by dream2nix project to use NixOS-like modules:

https://nix-community.github.io/dream2nix/modules.html

Since nix.dev is NixOS-blessed documentation, their best pratices advices are authoritative

Nothing in the nix.dev link indicates that meta = with lib; is unacceptable - that's a separate case from with lib; at the top of a file.

I do not consider this worth blocking a review on.

attributes not recognized by (say) stdenv.mkDerivation are treated as environment vars to the shell

Nit: basically all attributes are treated as envvars.

By using env you are explicitly saying this is an environment variable.

Agreed, the point of env is to be explicit about intention. If an attribute's purpose is to be used as an envvar and not a builder arg, then it should use env.

Nothing in the nix.dev link indicates that meta = with lib; is unacceptable - that's a separate case from with lib; at the top of a file.

with lib; around meta set invariably bring the issue of nested with.
The most typical usage is something like

meta = with lib; { maintainers = with maintainers; . . . ; platforms = with platforms; . . . ; }

And the nix.dev link says, among other things:

When more than one with used, it’s not clear anymore where the names are coming from.

Therefore, I believe that avoiding instances of meta = with lib; are worth removing and is in line with the arguments of that link.

Further, removing nested with and restricting with to single-line expressions is more manageable than removing with unconditionally, therefore being a good compromise.

I do not consider this worth blocking a review on.

Neither I.
But I will blame GitHub: it has no such a thing as a "yellow" unblocking review.
On the other hand, I have no power to block a PR indefinitely.

pkgs/by-name/ka/kana/package.nix

AndersonTorres · 2024-02-28T18:06:08Z

pkgs/by-name/ka/kana/package.nix

+    maintainers = with maintainers; [ aleksana ];
+    platforms = platforms.unix;


Suggested change

maintainers = with maintainers; [ aleksana ];

platforms = platforms.unix;

maintainers = with lib.maintainers; [ aleksana ];

platforms = lib.platforms.unix;

AndersonTorres · 2024-02-28T18:06:14Z

pkgs/by-name/ka/kana/package.nix

+    maintainers = with maintainers; [ aleksana ];
+    platforms = platforms.unix;
+  };
+}


Suggested change

}

})

KiaraGrouwstra · 2024-02-28T18:09:53Z

tested, the built application works fine :)

AndersonTorres

Break it on Darwin.

pkgs/by-name/ka/kana/package.nix

eclairevoyant

Builds and runs. Thanks for the contribution!

Regarding the other reviewer feedback:

There's currently no consensus to remove meta = with lib;
finalAttrs without some usage of said attrs outside of src or other FOD-related construct is also contentious
If we establish consensus, this can be addressed with a treewide change rather than blocking a package PR over it

eclairevoyant · 2024-03-11T07:54:58Z

pkgs/by-name/ka/kana/package.nix

+    domain = "gitlab.gnome.org";
+    owner = "fkinoshita";
+    repo = "Kana";
+    rev = "v${version}";


finalAttrs is preferable in cases where attributes of the derivation are actually used within the derivation - here the only places where it's used are in src and cargoDeps, which doesn't really save much effort when someone goes to override.

And this specific usage is at least somewhat contentious: see #293452 (comment)

I do not consider this worth blocking a review on.

eclairevoyant · 2024-03-11T08:01:14Z

pkgs/by-name/ka/kana/package.nix

+    gst-plugins-good
+  ]);
+
+  meta = with lib; {


Since nix.dev is NixOS-blessed documentation, their best pratices advices are authoritative

Nothing in the nix.dev link indicates that meta = with lib; is unacceptable - that's a separate case from with lib; at the top of a file.

I do not consider this worth blocking a review on.

attributes not recognized by (say) stdenv.mkDerivation are treated as environment vars to the shell

Nit: basically all attributes are treated as envvars.

By using env you are explicitly saying this is an environment variable.

Agreed, the point of env is to be explicit about intention. If an attribute's purpose is to be used as an envvar and not a builder arg, then it should use env.

ofborg bot added 8.has: package (new) 11.by: package-maintainer 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Feb 28, 2024

AndersonTorres suggested changes Feb 28, 2024

View reviewed changes

Aleksanaa force-pushed the kana branch 2 times, most recently from 82b7d1a to 48d80b8 Compare February 28, 2024 18:32

AndersonTorres suggested changes Mar 6, 2024

View reviewed changes

pkgs/by-name/ka/kana/package.nix Show resolved Hide resolved

Aleksanaa force-pushed the kana branch from 48d80b8 to c42c726 Compare March 6, 2024 14:33

kana: init at 1.4

37e3d62

Aleksanaa force-pushed the kana branch from c42c726 to 37e3d62 Compare March 6, 2024 18:52

Aleksanaa mentioned this pull request Mar 9, 2024

Documentation: style guide for use of with #292468

Open

lolbinarycat approved these changes Mar 9, 2024

View reviewed changes

delroth added the 12.approvals: 1 label Mar 9, 2024

eclairevoyant approved these changes Mar 11, 2024

View reviewed changes

eclairevoyant merged commit 7c5f52e into NixOS:master Mar 11, 2024
24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kana: init at 1.4 #292120

kana: init at 1.4 #292120

Aleksanaa commented Feb 28, 2024

AndersonTorres Feb 28, 2024

lolbinarycat Mar 5, 2024

AndersonTorres Mar 6, 2024

lolbinarycat Mar 6, 2024

eclairevoyant Mar 11, 2024

AndersonTorres Feb 28, 2024

Aleksanaa Feb 28, 2024

AndersonTorres Mar 6, 2024 •

edited

Loading

Aleksanaa Mar 6, 2024 •

edited

Loading

AndersonTorres Mar 9, 2024 •

edited

Loading

eclairevoyant Mar 11, 2024

AndersonTorres Mar 17, 2024

AndersonTorres Feb 28, 2024

AndersonTorres Feb 28, 2024

KiaraGrouwstra commented Feb 28, 2024

AndersonTorres left a comment

eclairevoyant left a comment •

edited

Loading

eclairevoyant Mar 11, 2024

eclairevoyant Mar 11, 2024

		maintainers = with maintainers; [ aleksana ];
		platforms = platforms.unix;

kana: init at 1.4 #292120

kana: init at 1.4 #292120

Conversation

Aleksanaa commented Feb 28, 2024

Description of changes

Things done

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndersonTorres Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

Aleksanaa Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

AndersonTorres Mar 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KiaraGrouwstra commented Feb 28, 2024

AndersonTorres left a comment

Choose a reason for hiding this comment

eclairevoyant left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndersonTorres Mar 6, 2024 •

edited

Loading

Aleksanaa Mar 6, 2024 •

edited

Loading

AndersonTorres Mar 9, 2024 •

edited

Loading

eclairevoyant left a comment •

edited

Loading