unifdef: 2.6 -> 2.12 #111275
unifdef: 2.6 -> 2.12 #111275
Conversation
|
|
||
| stdenv.mkDerivation rec { | ||
| name = "unifdef-2.6"; | ||
| name = "unifdef-2.12"; |
There was a problem hiding this comment.
| name = "unifdef-2.12"; | |
| pname = "unifdef"; | |
| version = "2.12"; |
|
|
||
| src = fetchurl { | ||
| url = "https://github.com/fanf2/unifdef/archive/${name}.tar.gz"; | ||
| sha256 = "1p5wr5ms9w8kijy9h7qs1mz36dlavdj6ngz2bks588w7a20kcqxj"; | ||
| url = "https://dotat.at/prog/unifdef/${name}.tar.xz"; |
There was a problem hiding this comment.
| url = "https://dotat.at/prog/unifdef/${name}.tar.xz"; | |
| url = "https://dotat.at/prog/unifdef/${pname}-${version}.tar.xz"; |
There was a problem hiding this comment.
I can do that. However, was there ever written a rationale for changing name to pname+version (when version is not actually needed separately from pname)? If so, you could have linked to it instead of suggesting specific changes. Especially if it was in Nixpkgs manual. (Currently it says to split name for convenience, but it is not convenient in case of unifdef.) I could argue both for and against using name: the argument for would be about the elegance of Eelco's original design when it perfectly fits unifdef and most projects on sourceforge, the argument against would be about consistency with many cases that do not fit with the name, especially most git projects.
There was a problem hiding this comment.
Yes, just do it.
I don't know if there is a written rationale. If there is one i will never find it because GitHub search is bad. So I can't link you anything. If you find it somewhere feel free to drop off a link and I will read it.
All new mkDerivation functions must use pname and version even if the name is used in the download. Old ones should be converted when doing cleanups. This makes updates and treewide changes easier and nixpkgs more uniform by not having edge cases which are avoidable. Same reason why version should not be in a let in or commands in pre and post phases should use multi line strings.
There was a problem hiding this comment.
OK, I have replaced name by pname and version. (I have not used pname in url because it is not useful, and fewer substitutions in url seem better.)
No, I have not seen this explained before. Now at least we can link to your comment.
You have convinced me, and I would ask the same of new contributors if I were to return to reviewing PRs (I am too busy with other stuff; thank you for doing it!), but until this is better codified (has an approved RFC, enters the manual) I can not think of this as actually necessary rather than as the best practice.
There was a problem hiding this comment.
I have not used pname in url because it is not useful, and fewer substitutions in url seem better.
Fine by me. I am not the biggest fan of using pname everywhere, either.
I can not think of this as actually necessary rather than as the best practice.
Most review comments I write are more best practice than fixing hard errors.
Motivation for this change
Needed to package cvise (its tests fail with 2.6).
Things done
Checked that it does not fail on the old xnu like 2.11 did in #16507 (comment)
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)nix path-info -Sbefore and after)