cup-docker{,-noserver}: init at 3.4.0#412710
Conversation
github-actions
bot
added
10.rebuild-darwin: 1-10
|
kuflierl
added
the
6.topic: docker tools
github-actions
bot
removed
the
6.topic: docker tools
kuflierl
changed the title
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/5550 |
kuflierl
added
the
6.topic: docker tools
github-actions
bot
removed
the
6.topic: docker tools
|
fix(cup-docker-noserver): remove dependency for the web component |
|
sorry to ping yall commiters, but this one went stale for like a month |
|
@pbsds second committer ping |
|
|
https://repology.org/project/cup-docker/versions seems the aur version has the wrong license https://aur.archlinux.org/packages/cup-docker |
|
builds on darwin |
emilylange
left a comment
emilylange
left a comment
There was a problem hiding this comment.
@pbsds what made you merge this as is?
| ++ lib.optional withServer [ | ||
| "server" | ||
| ]; |
There was a problem hiding this comment.
This results in
nix-repl> :p cup-docker.buildFeatures
[
"cli"
[ "server" ]
]
when it should have been either
++ lib.optionals withServer [
"server"
];or
++ lib.optional withServer "server";
There was a problem hiding this comment.
This is a genuine oversight, the issue is likely masked by builtins.toString. I'll make a fixup PR
There was a problem hiding this comment.
damn, probably made a typo. Thanks for noticing
| }; | ||
|
|
||
| meta = { | ||
| description = "a lightweight way to check for container image updates. written in Rust"; |
There was a problem hiding this comment.
This is not how we do meta.description.
meta.descriptionmust:
- Be short, just one sentence.
- Be capitalized.
- Not start with the definite or an indefinite article.
- Not start with the package name.
- More generally, it should not refer to the package name.
- Not end with a period (or any punctuation for that matter).
- Provide factual information.
- Avoid subjective language.
There was a problem hiding this comment.
The must seems to have slipped from my memory, thanks. I'll address this in a fixup
| tag = "v${version}"; | ||
| hash = "sha256-ciH3t2L2eJhk1+JXTqEJSuHve8OuVod7AuQ3iFWmKRE="; | ||
| }; | ||
| web = stdenvNoCC.mkDerivation (finalAttrs: { |
There was a problem hiding this comment.
Wherever you may or may not have copied this from, this is a bad FOD.
Builders like this are supposed to be multistaged. One FOD for downloading the files needed (node_modules) and then a regular input addressed derivation that depends on the FOD and other dependencies to build the actual thing.
Your approach here will inevitably break, but nobody will notice it because it's a FOD cached by cache.nixos.org.
There was a problem hiding this comment.
I let this slide since I remember frequently seeing this pattern with pnpm in the past. I even grepped for similar patterns for bun in nixpkgs before merging this. Yes, just because it has been allowed before doesn't mean it should be allowed. This s is however from May and the it still did build on both linux and darwin.
I'll be more careful in enforcing multistage fods going forward. 👍
There was a problem hiding this comment.
There is actually a very good reason for this. I had to add a per platform hash due to dependencies being a bit different on every platform (depending on the build platform) which had a tendency to break quite often. I have no way to practically determine the hash for every platform since i don't have a Darwin device at hand.
see the commit history in https://github.com/kuflierl/NUR/commits/main/pkgs/by-name/cu/cup-docker/package.nix especially kuflierl/NUR@813ef8e
There was a problem hiding this comment.
Thanks for shining some light on this, @kuflierl.
And yet, this is one of the reasons why you shouldn't yolo your own builder, at least not in nixpkgs.
You should have waited for a proper bun builder, e.g. #414837, to land and become available in nixpkgs instead of adding this FOD to nixpkgs.
It would have literally been better if you had generated your own package-lock.json and used buildNpmPackage for this, especially for this very simple package.json.
Even going the route with per-platform-hashes would have been better. If you don't have access to a darwin machine, that's fine, most maintainers don't. You could have simply skipped darwin support and called it a day.
This FOD is still a bad FOD which should not have been merged.
I even grepped for similar patterns for bun in nixpkgs before merging this.
The existing bun FODs in nixpkgs are different and multistaged, as they should be.
There was a problem hiding this comment.
What makes this one so special is that bun acts a bit like a static site generator. IE no binaries. While sub-optimal i have determined that the output is highly reproducible.
I have worked with pnpm packages in the past and do understand how that system works. I just don't want to roll my own custom solution when there is already a pr that is working on a general solution.
This really is just a temporary solution till the bun dep-getter is added.
You should have waited for a proper bun builder
We do have a lot of bun packages. I see no reason why i should wait for a pr that isn't even merge ready and could slug around for months to come.
| outputHashMode = "recursive"; | ||
| }); | ||
| in | ||
| rustPlatform.buildRustPackage { |
There was a problem hiding this comment.
The combination of one derivation using finalAttrs while the other isn't is confusing.
Why make web slightly more override if you won't be able to populate your overrides to the derivation that matters, rustPlatform.buildRustPackage?
There was a problem hiding this comment.
Mixed use of rec, let-in and finalAttrs derivations is not IMO a blocker. (finalAttrs: rec { ... } would be however.). I'll migrate it however to finalAttrs in my fixup. 👍
| substituteInPlace node_modules/.bin/{vite,tsc} \ | ||
| --replace-fail "/usr/bin/env node" "${nodejs-slim_latest}/bin/node" |
There was a problem hiding this comment.
This wouldn't be necessary if done with a proper builder.
There was a problem hiding this comment.
Neat, which one? I'm not familiar with bun and I'd love to learn, is there a existing builder that applies?
There was a problem hiding this comment.
Bun building is a bit broken in nixpkgs
| extraArgs = [ | ||
| "--subpackage" | ||
| "web" | ||
| ]; |
There was a problem hiding this comment.
This will not be able to update your web FOD.
There was a problem hiding this comment.
I'm used to seeing the passthru.web + nix-update --subpackage web pattern, and from my knowledge of the nix-update code I am under the impression that this works?
There was a problem hiding this comment.
I have tested this and it works
There was a problem hiding this comment.
No, I stand by this. This will not update your web FOD. nix-update is not able to update arbitrary outputHash in FODs like this. It's literally not implemented. See Mic92/nix-update#163. This isn't npmDeps/npmDepsHash.
Consider the following example updating this package from 3.2.3 to 3.4.0 (the version from this init PR):
diff --git a/pkgs/by-name/cu/cup-docker/package.nix b/pkgs/by-name/cu/cup-docker/package.nix
index db44b6a69986..69c4cb8c730e 100644
--- a/pkgs/by-name/cu/cup-docker/package.nix
+++ b/pkgs/by-name/cu/cup-docker/package.nix
@@ -10,12 +10,12 @@
}:
let
pname = "cup-docker";
- version = "3.4.0";
+ version = "3.2.3";
src = fetchFromGitHub {
owner = "sergi0g";
repo = "cup";
tag = "v${version}";
- hash = "sha256-ciH3t2L2eJhk1+JXTqEJSuHve8OuVod7AuQ3iFWmKRE=";
+ hash = "sha256-+z744Jeyh/1SGb3EeoI2bk3cfpkBIo2sczf2Ltv9JZ4=";
};
web = stdenvNoCC.mkDerivation (finalAttrs: {
pname = "${pname}-web";
@@ -47,7 +47,7 @@ let
cp -R ./dist $out
runHook postInstall
'';
- outputHash = "sha256-Ac1PJYmTQv9XrmhmF1p77vdXh8252hz0CUKxJA+VQR4=";
+ outputHash = "sha256-Ks4mNhMraeFtHYuV10IslkOgapkePaGdG5zJz9d8g0I=";
outputHashAlgo = "sha256";
outputHashMode = "recursive";
});
@@ -59,7 +59,7 @@ rustPlatform.buildRustPackage {
pname
;
- cargoHash = "sha256-L9zugOwlPwpdtjV87dT1PH7FAMJYHYFuvfyOfPe5b2k=";
+ cargoHash = "sha256-xzQfyBRXf5hANW8qNyKPiGldi3a7MXqWA0bDBovJrVk=";
buildNoDefaultFeatures = true;
buildFeatures =# nix-shell maintainers/scripts/update.nix --argstr package cup-docker
Going to be running update for following packages:
- cup-docker-3.2.3
Press Enter key to continue...
Running update for:
Enqueuing group of 1 packages
- cup-docker-3.2.3: UPDATING ...
- cup-docker-3.2.3: DONE.
Packages updated!which results in
diff --git a/pkgs/by-name/cu/cup-docker/package.nix b/pkgs/by-name/cu/cup-docker/package.nix
index 69c4cb8c730e..8d488957e3c6 100644
--- a/pkgs/by-name/cu/cup-docker/package.nix
+++ b/pkgs/by-name/cu/cup-docker/package.nix
@@ -10,12 +10,12 @@
}:
let
pname = "cup-docker";
- version = "3.2.3";
+ version = "3.4.0";
src = fetchFromGitHub {
owner = "sergi0g";
repo = "cup";
tag = "v${version}";
- hash = "sha256-+z744Jeyh/1SGb3EeoI2bk3cfpkBIo2sczf2Ltv9JZ4=";
+ hash = "sha256-ciH3t2L2eJhk1+JXTqEJSuHve8OuVod7AuQ3iFWmKRE=";
};
web = stdenvNoCC.mkDerivation (finalAttrs: {
pname = "${pname}-web";
@@ -59,7 +59,7 @@ rustPlatform.buildRustPackage {
pname
;
- cargoHash = "sha256-xzQfyBRXf5hANW8qNyKPiGldi3a7MXqWA0bDBovJrVk=";
+ cargoHash = "sha256-L9zugOwlPwpdtjV87dT1PH7FAMJYHYFuvfyOfPe5b2k=";
buildNoDefaultFeatures = true;
buildFeatures =Note how outputHash did not change?
# nix-build -A cup-docker.web
this derivation will be built:
/nix/store/whrcr4l5qlii1z45g36z4r6qzawg7vz2-cup-docker-web-3.4.0.drv
building '/nix/store/whrcr4l5qlii1z45g36z4r6qzawg7vz2-cup-docker-web-3.4.0.drv'...
Running phase: unpackPhase
unpacking source archive /nix/store/2b5i5i1gd4ypxrwidf25zxdkxca4ac58-source
source root is source/web
Running phase: patchPhase
[...]
patching script interpreter paths in /nix/store/g36fbnfxjbjih3vbqfwn02cp5dk6abpn-cup-docker-web-3.4.0
error: hash mismatch in fixed-output derivation '/nix/store/whrcr4l5qlii1z45g36z4r6qzawg7vz2-cup-docker-web-3.4.0.drv':
likely URL: (unknown)
specified: sha256-Ks4mNhMraeFtHYuV10IslkOgapkePaGdG5zJz9d8g0I=
got: sha256-Ac1PJYmTQv9XrmhmF1p77vdXh8252hz0CUKxJA+VQR4=
expected path: /nix/store/g36fbnfxjbjih3vbqfwn02cp5dk6abpn-cup-docker-web-3.4.0
got path: /nix/store/l1b9ngw1pq208mhg0a1p5xw6kgs1vmzv-cup-docker-web-3.4.0There was a problem hiding this comment.
This might be regression on my side. I had that working at some point.
| description = "a lightweight way to check for container image updates. written in Rust"; | ||
| homepage = "https://cup.sergi0g.dev"; | ||
| license = lib.licenses.agpl3Only; | ||
| platforms = lib.platforms.all; |
There was a problem hiding this comment.
rustPlatform.buildRustPackage defaults to this value.
There was a problem hiding this comment.
the guidelines state that meta.platforms must be set, we don't however have guidelines preferring implicit over explicit. I don't expect users to know that buildRustPackage and some other builders set meta.platforms while mkDerivation does not.
I likely weigh the cost of review cycles higher than others, and none of the small issues i located outweighed that cost. I'll be more rigorous of custom FODs going forward however 👍 |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/5644 |
5 participants
Homepage: https://github.com/sergi0g/cup
Issue: #410427
Source: https://github.com/kuflierl/NUR/blob/168edac19787f2fe08e65a208cf4e914726411f0/pkgs/by-name/cu/cup-docker/package.nix
Description: a lightweight way to check for container image updates. written in Rust
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.