-
-
Notifications
You must be signed in to change notification settings - Fork 18.3k
cup-docker{,-noserver}: init at 3.4.0 #412710
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| { | ||
| rustPlatform, | ||
| fetchFromGitHub, | ||
| lib, | ||
| stdenvNoCC, | ||
| bun, | ||
| nodejs-slim_latest, | ||
| nix-update-script, | ||
| withServer ? true, | ||
| }: | ||
| let | ||
| pname = "cup-docker"; | ||
| version = "3.4.0"; | ||
| src = fetchFromGitHub { | ||
| owner = "sergi0g"; | ||
| repo = "cup"; | ||
| tag = "v${version}"; | ||
| hash = "sha256-ciH3t2L2eJhk1+JXTqEJSuHve8OuVod7AuQ3iFWmKRE="; | ||
| }; | ||
| web = stdenvNoCC.mkDerivation (finalAttrs: { | ||
| pname = "${pname}-web"; | ||
| inherit version src; | ||
| impureEnvVars = lib.fetchers.proxyImpureEnvVars ++ [ | ||
| "GIT_PROXY_COMMAND" | ||
| "SOCKS_SERVER" | ||
| ]; | ||
| sourceRoot = "${finalAttrs.src.name}/web"; | ||
| nativeBuildInputs = [ | ||
| bun | ||
| nodejs-slim_latest | ||
| ]; | ||
| configurePhase = '' | ||
| runHook preConfigure | ||
| bun install --no-progress --frozen-lockfile | ||
| substituteInPlace node_modules/.bin/{vite,tsc} \ | ||
| --replace-fail "/usr/bin/env node" "${nodejs-slim_latest}/bin/node" | ||
|
Comment on lines
+35
to
+36
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This wouldn't be necessary if done with a proper builder.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Neat, which one? I'm not familiar with bun and I'd love to learn, is there a existing builder that applies?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bun building is a bit broken in nixpkgs |
||
| runHook postConfigure | ||
| ''; | ||
| buildPhase = '' | ||
| runHook preBuild | ||
| bun run build | ||
| runHook postBuild | ||
| ''; | ||
| installPhase = '' | ||
| runHook preInstall | ||
| mkdir -p $out/dist | ||
| cp -R ./dist $out | ||
| runHook postInstall | ||
| ''; | ||
| outputHash = "sha256-Ac1PJYmTQv9XrmhmF1p77vdXh8252hz0CUKxJA+VQR4="; | ||
| outputHashAlgo = "sha256"; | ||
| outputHashMode = "recursive"; | ||
| }); | ||
| in | ||
| rustPlatform.buildRustPackage { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The combination of one derivation using Why make
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mixed use of
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fair |
||
| inherit | ||
| src | ||
| version | ||
| pname | ||
| ; | ||
|
|
||
| cargoHash = "sha256-L9zugOwlPwpdtjV87dT1PH7FAMJYHYFuvfyOfPe5b2k="; | ||
|
|
||
| buildNoDefaultFeatures = true; | ||
| buildFeatures = | ||
| [ | ||
| "cli" | ||
| ] | ||
| ++ lib.optional withServer [ | ||
| "server" | ||
| ]; | ||
|
Comment on lines
+69
to
+71
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This results in when it should have been either ++ lib.optionals withServer [
"server"
];or ++ lib.optional withServer "server";
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a genuine oversight, the issue is likely masked by
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. damn, probably made a typo. Thanks for noticing |
||
|
|
||
| preConfigure = lib.optionalString withServer '' | ||
| cp -r ${web}/dist src/static | ||
| ''; | ||
|
|
||
| passthru = { | ||
| inherit web; | ||
| updateScript = nix-update-script { | ||
| extraArgs = [ | ||
| "--subpackage" | ||
| "web" | ||
| ]; | ||
|
Comment on lines
+80
to
+83
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will not be able to update your web FOD.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm used to seeing the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have tested this and it works
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, I stand by this. This will not update your web FOD. 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 # 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.0
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This might be regression on my side. I had that working at some point. |
||
| }; | ||
| }; | ||
|
|
||
| meta = { | ||
| description = "a lightweight way to check for container image updates. written in Rust"; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not how we do
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
| homepage = "https://cup.sergi0g.dev"; | ||
| license = lib.licenses.agpl3Only; | ||
| platforms = lib.platforms.all; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the guidelines state that |
||
| changelog = "https://github.com/sergi0g/cup/releases"; | ||
| mainProgram = "cup"; | ||
| maintainers = with lib.maintainers; [ | ||
| kuflierl | ||
| ]; | ||
| }; | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.jsonand usedbuildNpmPackagefor this, especially for this very simplepackage.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.
The existing bun FODs in nixpkgs are different and multistaged, as they should be.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.