bitwarden-cli: build from source

[dotlambda]

Description of changes

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• 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/)
• 23.05 Release Notes (or backporting 22.11 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.

[SuperSandro2000]

good idea to make updates easy but can't help you with npm workspaces :(

[dotlambda]

@winterqt I added a pretty rough way of handling workspace in buildNpmPackage. Let me know what you think.

[dotlambda]

One problem: This increases the output size from 85 MiB to 211 MiB. Running npm prune --workspace apps/cli must be suboptimal.

[SuperSandro2000]

I inspected the node_module directory of inside bitwarden/cli and the biggest directory, angular, was not there before. I think the cli is not using that at all and out vendoring just picks up everything in the repo, not just the dependencies of the workspace. Maybe you also need to adjust the vendoring part? Not sure if that happens in https://github.com/dotlambda/nixpkgs/blob/bitwarden-cli-source/pkgs/build-support/node/build-npm-package/hooks/npm-config-hook.sh

[lilyinstarlight]

Overall I think adding an npmWorkspace arg can be workable for now to deal with workspaces. Please add it to the documentation too, if you can

I've also noticed npm sometimes tries to reach out to the registry even when the entire lockfile is cached when workspaces are involved, so if doing it like this avoids that, then great! I was dreading eventually having to look into that

[lilyinstarlight]

#233804 has been merged adding npmWorkspace as a buildNpmPackage arg, so if this could be rebased on top of that, then this should be good for merge. Thank you for your work @dotlambda!

[dotlambda]

I inspected the node_module directory of inside bitwarden/cli and the biggest directory, angular, was not there before. I think the cli is not using that at all and out vendoring just picks up everything in the repo, not just the dependencies of the workspace. Maybe you also need to adjust the vendoring part? Not sure if that happens in https://github.com/dotlambda/nixpkgs/blob/bitwarden-cli-source/pkgs/build-support/node/build-npm-package/hooks/npm-config-hook.sh

Running npm prune within apps/cli doesn't seem to work so I'm unsure how to correctly get rid of the unnecessary dependencies.

[lilyinstarlight]

Running npm prune within apps/cli doesn't seem to work so I'm unsure how to correctly get rid of the unnecessary dependencies.

Yeah the prune step should be handled by install hook now anyway. The fact that it leaves root-level project deps in that aren't necessary definitely seems like an npm bug, but I'm not sure how to coerce it to Do The Right Thing and also not quite sure in the npm sources where the bug is

I'll assume the closure increase is okay enough for now, but I'll try to either fix the npm bug or come up with a workaround for the install hook to successfully prune workspace deps (now whether I'll end up needing to temporarily rename the root-level packages.json or what, we'll see)

[lilyinstarlight]

Quick nit to fix darwin build, then this PR looks good to me

Thank you so much for this work and the work on npm workspaces in our tooling!

[lilyinstarlight]

Looks good to me now, thank you so much!

I'll open a new issue to separately investigate why npm is buggy and not removing top-level deps when pruning (there is an upstream bug about it, but we should be able to work around it, since the npm folks generally aren't interested in fixing these sorts of bugs...)

[dotlambda]

Should we add an alias for nodePackages.bitwarden-cli? What about nodePackages."@bitwarden/cli"?
See #245337

[SuperSandro2000]

Yes and remove it from the nodePackage generation. There are some prior example of aliases in that same directory.

[dotlambda]

remove it from the nodePackage generation

That already happened in this PR. It was just left in node-packages.nix to avoid merge conflicts.