Skip to content

cup-docker: drop#424038

Open
pbsds wants to merge 1 commit intoNixOS:masterfrom
pbsds:fix-bun-1752150737
Open

cup-docker: drop#424038
pbsds wants to merge 1 commit intoNixOS:masterfrom
pbsds:fix-bun-1752150737

Conversation

@pbsds
Copy link
Member

@pbsds pbsds commented Jul 10, 2025

@emilylange if you want me to split the FOD then please let me know, and i'll do so later tonight.
@kuflierl please test this

edit: closes #426967

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/)
  • Nixpkgs 25.11 Release Notes (or backporting 25.05 Nixpkgs Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
  • NixOS 25.11 Release Notes (or backporting 25.05 NixOS Release notes)
    • (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, pkgs/README.md, maintainers/README.md and other contributing documentation in corresponding paths.

Add a 👍 reaction to pull requests you find important.

@pbsds pbsds mentioned this pull request Jul 10, 2025
16 tasks
@nix-owners nix-owners bot requested a review from kuflierl July 10, 2025 12:54
@nixpkgs-ci nixpkgs-ci bot added 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. labels Jul 10, 2025
Copy link
Member

@kuflierl kuflierl left a comment

Choose a reason for hiding this comment

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

Fair changes, though i am unsure if we need that many spaces in the phases. May need a bit of work.

@pbsds pbsds force-pushed the fix-bun-1752150737 branch from 1eb3f32 to ef36f56 Compare July 10, 2025 13:37
@nixpkgs-ci nixpkgs-ci bot added 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Jul 10, 2025
@pbsds
Copy link
Member Author

pbsds commented Jul 10, 2025

@pbsds pbsds requested a review from emilylange July 10, 2025 14:30
Copy link
Member

@emilylange emilylange left a comment

Choose a reason for hiding this comment

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

@emilylange if you want me to split the FOD then please let me know, and i'll do so later tonight.

Yes, please do something about the FOD, as per #412710 (comment)

Comment on lines 48 to 50
Copy link
Member

Choose a reason for hiding this comment

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

patchShebangs does work, just not on a symlink:

Suggested change
# patchShebangs doesn't work
substituteInPlace node_modules/.bin/{vite,tsc} \
--replace-fail "/usr/bin/env node" "${nodejs-slim_latest}/bin/node"
substituteInPlace node_modules

@kuflierl
Copy link
Member

If you want to split up the FOD, you could use my previous "solution" https://github.com/kuflierl/NUR/blob/dd56379ca544011e48bd8ffe134d5198555b28c3/pkgs/by-name/cu/cup-docker/package.nix. This is an old variation of my code where i still had them separated.

@kuflierl kuflierl self-requested a review July 11, 2025 04:18
@pbsds
Copy link
Member Author

pbsds commented Jul 14, 2025

I had some time today so I took a crack at it.

I first tried to revert to the setup in #424038 (comment), but putting four FODs in the cache just to produce the same dist web folder on all platforms didn't sit right with me. Nor do I particularly like putting a npm package.lock file in the nixpkgs tarball.

Bun doesn't seem to have any stable offline cache capability like npm and yarn, and cannot be forced to download all artifacts for all platforms into its local pullthrough cache. I did find bun install --yarn which converts the bun lock file to a yarn lock file, which is what the diff below is based on, however then yarn install fails with error Couldn't find any versions for "type-check" that matches "~0.4.0" in our cache (possible versions are ""). This is usually caused by a missing entry in the lockfile, running Yarn without the --offline flag may help fix this issue.... This is despite https___registry.npmjs.org_type_check___type_check_0.4.0.tgz having been downloaded

Current progress:

diff --git a/pkgs/by-name/cu/cup-docker/package.nix b/pkgs/by-name/cu/cup-docker/package.nix
index 91b60a356514..f943203d2a41 100644
--- a/pkgs/by-name/cu/cup-docker/package.nix
+++ b/pkgs/by-name/cu/cup-docker/package.nix
@@ -2,9 +2,10 @@
   rustPlatform,
   fetchFromGitHub,
   lib,
-  stdenvNoCC,
+  fetchYarnDeps,
+  yarnConfigHook,
+  yarnBuildHook,
   bun,
-  nodejs-slim_latest,
   nix-update-script,
   withServer ? true,
 }:
@@ -20,58 +21,22 @@ rustPlatform.buildRustPackage (finalAttrs: {
     hash = "sha256-ciH3t2L2eJhk1+JXTqEJSuHve8OuVod7AuQ3iFWmKRE=";
   };
 
-  web = lib.optionalDrvAttr withServer (
-    stdenvNoCC.mkDerivation {
-      pname = "${finalAttrs.pname}-web";
-
-      inherit (finalAttrs) version src;
-      sourceRoot = "${finalAttrs.src.name}/web";
-
-      outputHash = "sha256-Ac1PJYmTQv9XrmhmF1p77vdXh8252hz0CUKxJA+VQR4=";
-      outputHashAlgo = "sha256";
-      outputHashMode = "recursive";
-
-      impureEnvVars = lib.fetchers.proxyImpureEnvVars ++ [
-        "GIT_PROXY_COMMAND"
-        "SOCKS_SERVER"
-      ];
-
-      nativeBuildInputs = [
-        bun
-        nodejs-slim_latest
-      ];
-
-      configurePhase = ''
-        runHook preConfigure
-
-        bun install --no-progress --frozen-lockfile
-        # patchShebangs doesn't work
-        substituteInPlace node_modules/.bin/{vite,tsc} \
-          --replace-fail "/usr/bin/env node" "${nodejs-slim_latest}/bin/node"
-
-        runHook postConfigure
-      '';
-
-      buildPhase = ''
-        runHook preBuild
-
-        bun run build
-
-        runHook postBuild
-      '';
-
-      installPhase = ''
-        runHook preInstall
-
-        mkdir -p $out/dist
-        cp -R ./dist $out
+  cargoHash = "sha256-L9zugOwlPwpdtjV87dT1PH7FAMJYHYFuvfyOfPe5b2k=";
 
-        runHook postInstall
-      '';
-    }
-  );
+  yarnOfflineCache = fetchYarnDeps {
+    name = "${finalAttrs.pname}-yarn-deps-${finalAttrs.version}";
+    inherit (finalAttrs) src;
+    sourceRoot = "${finalAttrs.src.name}/web";
+    hash = "sha256-4onJtIqZtVJ1m5gKEznZJ6SmCuIwHRMAMUJulwHA2v8=";
+    preBuild = ''
+      ${lib.getExe bun} install --no-progress --frozen-lockfile --dry-run --yarn
+    '';
+  };
 
-  cargoHash = "sha256-L9zugOwlPwpdtjV87dT1PH7FAMJYHYFuvfyOfPe5b2k=";
+  nativeBuildInputs = lib.optionals withServer [
+    yarnConfigHook
+    yarnBuildHook
+  ];
 
   buildNoDefaultFeatures = true;
   buildFeatures =
@@ -82,24 +47,42 @@ rustPlatform.buildRustPackage (finalAttrs: {
       "server"
     ];
 
-  preConfigure = lib.optionalString withServer ''
-    cp -r $web/dist src/static
+  dontYarnInstallDeps = true; # wrong work dir
+  postConfigure = lib.optionalString withServer ''
+    pushd web
+
+    # this skips the out-of-date check, so we put the version in yarnOfflineCache.name
+    cp $yarnOfflineCache/yarn.lock .
+    chmod +w yarn.lock
+    yarnConfigHook
+
+    popd
+  '';
+
+  dontYarnBuild = true; # wrong work dir
+  preBuild = lib.optionalString withServer ''
+    pushd web
+
+    yarnBuildHook
+    cp -r dist ../src/static
+
+    popd
   '';
 
   passthru = {
-    updateScript = nix-update-script {
-      extraArgs = [
-        "--subpackage"
-        "web"
-      ];
-    };
+    updateScript = nix-update-script { };
   };

Any ideas? If no novel ideas are found, should we (a) hash four FODs, (b) check in huge lock file, (c) leave it and migrate the moment #414837 is merged, or (d) drop the package?

@kuflierl
Copy link
Member

kuflierl commented Jul 15, 2025

Any ideas? If no novel ideas are found, should we (a) hash four FODs, (b) check in huge lock file, (c) leave it and migrate the moment #414837 is merged, or (d) drop the package?

I would prefer the options in the following order:

  1. C: Best maintainability currently, it works even if it doesn't follow best practices.
  2. A: While less optimal, this would still acceptably maintainable .
  3. B: Gigantic lock files in nixpkgs already are a scourge since they just grow the total repo size. It may need to be done if it has to but don't expect me to support it.
  4. D: I don't see a reason why we should drop a perfectly functional package (even if the packaging is suboptimal). Upstream is still well maintained.

Migration to #414837 the moment it is merged is probably a given from me.

I had previously looked into the yarn solution but i didn't really have the motivation to keep running against that wall. It certainly is promising but something tells me that either the original lockfiles are too old, the yarn generation has an issue, or we are missing something of substantial significance.

@nixpkgs-ci nixpkgs-ci bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 24, 2025
@emilylange
Copy link
Member

I think the idea with bun install --yarn is sweet, but it does add additional fragileness, besides, well, not working currently.

If no novel ideas are found, should we (a) hash four FODs, (b) check in huge lock file, (c) leave it and migrate the moment #414837 is merged, or (d) drop the package?

I side with option (b) as the resulting lockfile is not as huge as one might expect and this is a very acceptable reason for inclusion of such lockfile.

[...]
B: Gigantic lock files in nixpkgs already are a scourge since they just grow the total repo size. It may need to be done if it has to but don't expect me to support it.
D: I don't see a reason why we should drop a perfectly functional package (even if the packaging is suboptimal). Upstream is still well maintained.

To be entirely frank, this is nixpkgs and not NUR. I am of the very strong opinion that the current FOD must go and that the FOD should not have been added. nixpkgs has (higher) standards and processes that may result in a technically working and arguably very space efficient FOD to not be included for various reasons. Not every package has to be included in nixpkgs. Our pkgs/README.md even says

Luckily it's pretty easy to maintain your own package set with Nix, which can then be added to the Nix User Repository project.

and

How realistic is it that it will be used by other people? It's good that nixpkgs caters to various niches, but if it's a niche of 5 people it's probably too small. A good estimate is checking upstream issues and pull requests, or other software repositories. Library packages should have at least one dependent.

I did not mention this point in my other comments regard this package yet, but I genuinely don't see why a niche software made for Docker, that requires Docker to work, and is packages as Docker image by upstream, has to be included in nixpkgs despite packaging issues that are nixpkgs' fault.

And even when ignoring the whole "but if it's a niche of 5 people it's probably too small" policy and working towards keeping this is nixpkgs by suggesting to vendor a lockfile until some proper bun fetcher is made available, its sole maintainer says that they would effectively stop maintaining it ("but don't expect me to support it").

I do think option (d) is reasonable enough to consider. At least until there is a proper bun fetcher in nixpkgs. Though it should be noted that this package might still be too niche and I personally don't understand the benefits of having this particular package packaged outside a Dockerfile. Maybe someone else could shine some light on this. I would appreciate it.


I am sorry if this somehow worsens your image of nixpkgs or lowers your desire to contribute to nixpkgs. It's just that nixpkgs has a lot of issues and while yes, your FOD works and is creative, it's not how nixpkgs strives to be. If we keep this FOD, others will copy it and others will start to argue "Well look at this FOD that's in nixpkgs. Then why am I not allowed to do the same?"

@kuflierl
Copy link
Member

kuflierl commented Aug 8, 2025

And even when ignoring the whole "but if it's a niche of 5 people it's probably too small" policy and working towards keeping this is nixpkgs by suggesting to vendor a lockfile until some proper bun fetcher is made available, its sole maintainer says that they would effectively stop maintaining it ("but don't expect me to support it").

I actually meant "don't expect me to be favourable towards this change". Sorry for my bad choice of words. In a way, I consider generating and keeping lock-files in nixpkgs to be a bit of an issue, since it

  1. Increases the size of nixpkgs
  2. Makes updating the package non-trivial due to custom lockfile generation
  3. Requires bloated auto update scripts (that still need to be written)
  4. Doesn't patch but overwrites something in the repo.

If it needs to be done before the PR for the bun fetcher I would still keep maintaining it but updates might take longer.

@pbsds
Copy link
Member Author

pbsds commented Aug 12, 2025

I tried my hand on route B, but it resulted in a 4829-line lock file and bun proved unable to use the resulting node_modules. I'm now working long days until at least october and I'm all out of spoons, so I propose option D... :(

@pbsds pbsds force-pushed the fix-bun-1752150737 branch from ef36f56 to b6fb9c7 Compare August 12, 2025 23:22
@pbsds pbsds changed the title cup-docker: refactor, fixup meta cup-docker: drop Aug 12, 2025
@nixpkgs-ci nixpkgs-ci bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. and removed 2.status: merge conflict This PR has merge conflicts with the target branch 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Aug 12, 2025
Copy link
Member

@kuflierl kuflierl left a comment

Choose a reason for hiding this comment

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

No, I still think C is just fine

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 23, 2025
The web FOD was unsound, see NixOS#424038 for context
@pbsds pbsds force-pushed the fix-bun-1752150737 branch from b6fb9c7 to 6ca1d4f Compare August 26, 2025 15:12
@nixpkgs-ci nixpkgs-ci bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 26, 2025
@nixpkgs-ci nixpkgs-ci bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 8, 2025
@mdaniels5757 mdaniels5757 added the 8.has: clean-up This PR removes packages or removes other cruft label Oct 25, 2025
@emilylange
Copy link
Member

Any update on this?

Copy link
Member

@Eveeifyeve Eveeifyeve left a comment

Choose a reason for hiding this comment

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

Pr in progress for fetch and config deps hooks: #376299 that does this node_modules automatically. The pr I linked will close the fod part of this pr. I am also happy to do a seperate pr to solve #426967 after the bun fetch deps and config hook is done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2.status: merge conflict This PR has merge conflicts with the target branch 8.has: clean-up This PR removes packages or removes other cruft 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants