Skip to content
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

wrangler: init at 3.62.0 #322573

Merged
merged 4 commits into from
Jul 12, 2024
Merged

wrangler: init at 3.62.0 #322573

merged 4 commits into from
Jul 12, 2024

Conversation

dezren39
Copy link
Contributor

@dezren39 dezren39 commented Jun 26, 2024

Description of changes

updates wrangler to be standalone package.

previously it was available but only as part of the nodePackages set.

initially i had just updated hashes because wrangler build broke.

wrangler build had other issues even after hash update,

and it was also suggested that it's preferred to use the new way to handle npm builds.

wrangler is published to npm without a package-lock,

it uses a pnpm build with workspaces as part of the cloudflare/workers-sdk monorepo.

some dependencies of wrangler are part of the monorepo and are unavailable on npm.

the pnpm helper functions such as pnpm.fetchDeps do not support monorepos directly.

this deployment works around that by copying a large part of the pnpm cache (hard links in node_modules into the cache) directly into the workflow.

because pnpm dependency resolution can sometimes pull dev dependencies and other utilities,

a few dev dependencies that are also binaries are removed in the build script.

  • typescript
  • eslint
  • prettier

there may be other dependencies which could conflict other nixos builds.

fixes #322571

may relate to #319134

Things done

i confirmed this builds on my nixos framework 13 intel 11 gen,

as opposed to before when build threw.

  • 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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (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.

Add a 👍 reaction to pull requests you find important.

@NixOSInfra NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Jun 26, 2024
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Jun 26, 2024
@dezren39
Copy link
Contributor Author

dezren39 commented Jul 1, 2024

@SuperSandro2000 @lelgenio @a-kenji @friedow can you please review

@dezren39
Copy link
Contributor Author

dezren39 commented Jul 1, 2024

this makes wrangler build as far as nix is concerned but the binary itself still has some issues

Error: Cannot find module 'unenv'

Eventually this probably should be resolved too if it happens on other machines, but I'm not entirely sure the reasons/solution for that issue.

@dezren39 dezren39 changed the title update workerd hash workerd: update hash Jul 1, 2024
Copy link
Contributor

@lelgenio lelgenio left a comment

Choose a reason for hiding this comment

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

You could also update the hash for the other systems:
Using nix-hash to-sri --type sha512 "$(nix-prefetch-url --type sha512 <URL>)", I get:

System Hash
linux-64 sha512-2zDcadR7+Gs9SjcMXmwsMji2Xs+yASGNA2cEHDuFc4NMUup+eL1mkzxc/QzvFjyBck98e92rBjMZt2dVscpGKg==
linux-arm64 sha512-7y41rPi5xmIYJN8CY+t3RHnjLL0xx/WYmaTd/j552k1qSr02eTE2o/TGyWZmGUC+lWnwdPQJla0mXbvdqgRdQg==
darwin-64 sha512-YanZ1iXgMGaUWlleB5cswSE6qbzyjQ8O7ENWZcPAcZZ6BfuL7q3CWi0t9iM1cv2qx92rRztsRTyjcfq099++XQ==
darwin-arm64 sha512-bRe/y/LKjIgp3L2EHjc+CvoCzfHhf4aFTtOBkv2zW+VToNJ4KlXridndf7LvR9urfsFRRo9r4TXCssuKaU+ypQ==

@dezren39
Copy link
Contributor Author

dezren39 commented Jul 1, 2024

I didn't know you could do that. I'll update

@ofborg ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin labels Jul 1, 2024
@SuperSandro2000
Copy link
Member

It seems that workerd is important to you. Please move it out of nodePackages to prevent such regressions in the future. see #229475

@dezren39 dezren39 changed the title workerd: update hash wrangler: use buildNpmPackage Jul 1, 2024
@github-actions github-actions bot added the 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` label Jul 1, 2024
@dezren39 dezren39 changed the title wrangler: use buildNpmPackage wrangler: use pnpm Jul 1, 2024
@ofborg ofborg bot added the 11.by: package-maintainer This PR was created by the maintainer of the package it changes label Jul 1, 2024
@dezren39 dezren39 changed the title wrangler: use pnpm wrangler: use pnpm.fetchDeps Jul 1, 2024
@dezren39
Copy link
Contributor Author

dezren39 commented Jul 1, 2024

issues with pnpm monorepo, looking at suggestions from #316908 which may have a path forward

@ofborg ofborg bot added 8.has: clean-up 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin and removed 10.rebuild-darwin: 1 10.rebuild-darwin: 1-10 labels Jul 1, 2024
@seanrmurphy
Copy link
Contributor

issues with pnpm monorepo, looking at suggestions from #316908 which may have a path forward

I was looking at this a week or two ago - this is what I've come up with - this resulted in a successful wrangler build but when I tried to run it the paths in the script were not correct and it prob didn't know where to look for all pnpm dependencies - I guess they should somehow also be copied to $out somewhere. It's not working yet, but it might be helpful:

{
  lib,
  stdenv,
  fetchFromGitHub,
  pnpm_9,
  nodejs,
  npmHooks,
  writeScriptBin,
}:

stdenv.mkDerivation (finalAttrs: {
  pname = "wrangler";
  version = "3.62.0";

  src = fetchFromGitHub {
    owner = "cloudflare";
    repo = "workers-sdk";
    rev = "wrangler@${finalAttrs.version}";
    hash = "sha256-/4iIkvSn85fkRggmIha2kRlW0MEwvzy0ZAmIb8+LpZQ=";
  };

  nativeBuildInputs = [
    nodejs
    pnpm_9.configHook
  ];

  pnpmDeps = pnpm_9.fetchDeps {
    inherit (finalAttrs) pname version src;
    hash = "sha256-aTTaiGXm1WYwmy+ljUC9yO3qtvN20SA+24T83dWYrI0=";
  };

  # @cloudflare/vitest-pool-workers wanted to run a server as part of the build process 
  # so I simply removed it 
  postBuild = ''
    rm -fr packages/vitest-pool-workers
    NODE_ENV="production" pnpm run build
  '';

  # this does install what gets executed, but it has a lot of dependencies on stuff which was 
  # imported so it does not work as is...
  # there seem to be a couple of issues:
  # 1. the path which is used during the build process seems to be part of the executable - sigh
  # 2. the pnpm stuff which was pulled in during the build process is not put anywhere...
  installPhase = ''
    #find . -name wrangler -print
    runHook preInstall
    mkdir -p $out/bin
    cp -r node_modules/.pnpm/node_modules/.bin/* $out/bin
  '';

})

@seanrmurphy
Copy link
Contributor

I did a little more on this and this variant seems to somehow work (in the sense that it is possible to print version, login and list dbs) but it feels very brittle and I really don't have much understanding of any model assumed by pnpm (ie what should go where) so I guess it can be done better. In particular, this copies an entire node_modules dir which is used in the build process across a monorepo which prob is adding too much to the resulting package.

Might be useful/helpful in any case...

{
  lib,
  pkgs,
  stdenv,
  fetchFromGitHub,
  pnpm_9,
  nodejs,
  npmHooks,
  writeScriptBin,
}:

stdenv.mkDerivation (finalAttrs: {
  pname = "wrangler";
  version = "3.62.0";

  src = fetchFromGitHub {
    owner = "cloudflare";
    repo = "workers-sdk";
    rev = "wrangler@${finalAttrs.version}";
    hash = "sha256-/4iIkvSn85fkRggmIha2kRlW0MEwvzy0ZAmIb8+LpZQ=";
  };

  nativeBuildInputs = [
    nodejs
    pnpm_9.configHook
  ];

  pnpmDeps = pnpm_9.fetchDeps {
    inherit (finalAttrs) pname version src;
    hash = "sha256-aTTaiGXm1WYwmy+ljUC9yO3qtvN20SA+24T83dWYrI0=";
  };

  # @cloudflare/vitest-pool-workers wanted to run a server as part of the build process 
  # so I simply removed it 
  postBuild = ''
    rm -fr packages/vitest-pool-workers
    NODE_ENV="production" pnpm run build
    # pnpm --offline deploy --frozen-lockfile  --ignore-script  --filter=bash-language-server server-deploy
  '';

  # this was taken from a previous script which was generated somehow
  wranglerScript = pkgs.writeText "wrangler" ''
        #!/bin/sh
        basedir=$(dirname "$(echo "$0" | sed -e 's,\\,/,g')")

        case `uname` in
            *CYGWIN*) basedir=`cygpath -w "$basedir"`;;
        esac

        if [ -z "$NODE_PATH" ]; then
          export NODE_PATH="WRANGLER_PATH/lib/node_modules:WRANGLER_PATH/lib/packages/wrangler/node_modules"
        else
          export NODE_PATH="WRANGLER_PATH/lib/node_modules:WRANGLER_PATH/lib/packages/wrangler/node_modules:$NODE_PATH"
        fi
        if [ -x "$basedir/node" ]; then
          exec "$basedir/node"  "WRANGLER_PATH/lib/packages/wrangler/bin/wrangler.js" "$@"
        else
          exec node  "WRANGLER_PATH/lib/packages/wrangler/bin/wrangler.js" "$@"
        fi
      '';

  # I'm sure this is suboptimal but it seems to work. Points:
  # - when build is run in the original repo, no specific executable seems to be generated; you run the resulting build with pnpm run start 
  # - this means we need to add a dedicated script - perhaps it is possible to create this frok the workers-sdk dir, but I don't know how to do this
  # - the bulid process builds a version of miniflare which is used by wrangler; for this reason, the miniflare package is copied also
  # - pnpm stores all content in the top-level node_modules directory, but it is linked to from a node_modules directory inside wrangler
  # - as they are linked via symlinks, the relative location of them on the filesystem should be maintained
  installPhase = ''
    runHook preInstall
    mkdir -p $out/bin $out/lib $out/lib/packages/wrangler
    cp -r node_modules $out/lib
    cp -r packages/wrangler/bin $out/lib/packages/wrangler
    cp -r packages/wrangler/wrangler-dist $out/lib/packages/wrangler
    cp -r packages/wrangler/node_modules $out/lib/packages/wrangler
    cp -r packages/miniflare $out/lib/packages/wrangler
    cp $wranglerScript $out/bin/wrangler
    chmod a+x $out/bin/wrangler
    substituteInPlace $out/bin/wrangler --replace-warn /bin/sh ${pkgs.bash}/bin/sh
    substituteInPlace $out/bin/wrangler --replace-warn WRANGLER_PATH $out 
  '';

})

@dezren39
Copy link
Contributor Author

dezren39 commented Jul 3, 2024

I did a little more on this and this variant seems to somehow work (in the sense that it is possible to print version, login and list dbs) but it feels very brittle and I really don't have much understanding of any model assumed by pnpm (ie what should go where) so I guess it can be done better. In particular, this copies an entire node_modules dir which is used in the build process across a monorepo which prob is adding too much to the resulting package.

getting closer, now i'm looking at this when i run the derivation as part of a larger nixos deploy

 > error: collision between `/nix/store/8h5s7xk9pq5i0nqv5qhs6pn4vw8mrj5f-typescript-5.5.2/lib/node_modules/typescript/package.json' and `/nix/store/pxji292jk5b1mqz45vyfhhgi9x8j723r-wrangler-3.62.0/lib/node_modules/typescript/package.json'

i'm guessing i pull in typescript as part of the build or something, but i'm not sure if that's a good solution. there are probably other npm deps after typescript. i'm definitely way out past my ski's here, but i'll try hacking a bit at this on my laptop for a while.

@dezren39
Copy link
Contributor Author

dezren39 commented Jul 8, 2024

@seanrmurphy it still builds when i removed autopatch altogether and that seems to have worked.. but should i have just removed the preBuild step? what do you think? guessing one of those subcommands might fail if we don't have the autopatch

@seanrmurphy
Copy link
Contributor

@seanrmurphy it still builds when i removed autopatch altogether and that seems to have worked.. but should i have just removed the preBuild step? what do you think? guessing one of those subcommands might fail if we don't have the autopatch

The autopatch is necessary to run workerd - without the autopatch, it will not be able to find the libraries necessary to run workerd and the error I highlighted above should manifest when wrangler dev is run.

It can be built without the autopatch, because the 'build' is mostly just copying and downloading files afaict. I believe functionalities that involve querying the API should prob be fine without the autopatching but anything which is dependent on the binaries could fail.

I think the preBuild step can be removed - it's somehow a directive to tell autopatch where to look for files; by default it looks under lib but it was not clear to me if it performs a full search under this directory or simply looks one level deep. For this reason, I explicitly specified a full path. When running it, it was clear that it does a full tree search under lib and hence I don't think it's necessary to specify this path.

@dezren39
Copy link
Contributor Author

dezren39 commented Jul 8, 2024

@seanrmurphy ill add back autopatch, makes sense to me

Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

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

Looks almost perfect! Well done :).

pkgs/by-name/wr/wrangler/package.nix Outdated Show resolved Hide resolved
@dezren39 dezren39 force-pushed the patch-2 branch 2 times, most recently from c91a862 to 18338bc Compare July 10, 2024 08:31
pkgs/by-name/wr/wrangler/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/wr/wrangler/package.nix Show resolved Hide resolved
pkgs/by-name/wr/wrangler/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/wr/wrangler/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/wr/wrangler/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/wr/wrangler/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/wr/wrangler/package.nix Show resolved Hide resolved
@dezren39 dezren39 force-pushed the patch-2 branch 2 times, most recently from 987add4 to 3554346 Compare July 10, 2024 08:41
@doronbehar
Copy link
Contributor

Also please don't push so frequently - you are exhausting CI. Push only after you have tested it builds locally and after you have gone through all the unresolved review comments at least twice and made sure they are all addressed.

@dezren39 dezren39 force-pushed the patch-2 branch 2 times, most recently from 48bf9a2 to 8cbda5f Compare July 10, 2024 08:56
@doronbehar
Copy link
Contributor

Also please don't push so frequently - you are exhausting CI. Push only after you have tested it builds locally and after you have gone through all the unresolved review comments at least twice and made sure they are all addressed.

@dezren39 you are not behaving as I asked you. Not only that, you are marking conversations as resolved without really resolving them. I won't be helping you to get this in if you won't behave.

pkgs/by-name/wr/wrangler/package.nix Outdated Show resolved Hide resolved
@DataHearth
Copy link
Contributor

Looks awesome 👏! Hopes this can be shipped soon 👌👌

@doronbehar
Copy link
Contributor

Result of nixpkgs-review pr 322573 run on x86_64-linux 1

1 package built:
  • wrangler

@doronbehar doronbehar merged commit 82cc19f into NixOS:master Jul 12, 2024
27 checks passed
@seanrmurphy seanrmurphy mentioned this pull request Jul 12, 2024
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nodejs 8.has: clean-up 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build failure: wrangler
9 participants