Skip to content

bitwarden-cli: build from source#218923

Merged
lilyinstarlight merged 1 commit intoNixOS:masterfrom
dotlambda:bitwarden-cli-source
Jul 23, 2023
Merged

bitwarden-cli: build from source#218923
lilyinstarlight merged 1 commit intoNixOS:masterfrom
dotlambda:bitwarden-cli-source

Conversation

@dotlambda
Copy link
Member

@dotlambda dotlambda commented Mar 1, 2023

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:
  • 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.

dotlambda
dotlambda commented Mar 1, 2023
View reviewed changes
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. labels Mar 1, 2023
SuperSandro2000
SuperSandro2000 reviewed Mar 1, 2023
View reviewed changes
Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

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

@dotlambda dotlambda added the 6.topic: nodejs Node.js is a free, open-source, cross-platform JavaScript runtime environment label Mar 22, 2023
@dotlambda dotlambda force-pushed the bitwarden-cli-source branch from 60e4541 to ba22689 Compare March 22, 2023 02:34
@dotlambda
Copy link
Member Author

dotlambda commented Mar 22, 2023

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

@dotlambda dotlambda marked this pull request as ready for review March 22, 2023 02:35
@dotlambda dotlambda requested a review from winterqt as a code owner March 22, 2023 02:35
@dotlambda
Copy link
Member Author

dotlambda commented Mar 22, 2023

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

SuperSandro2000
SuperSandro2000 reviewed Mar 26, 2023
View reviewed changes
@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Mar 26, 2023

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
lilyinstarlight reviewed Mar 28, 2023
View reviewed changes
Copy link
Member

@lilyinstarlight lilyinstarlight left a comment •
edited
Loading

Choose a reason for hiding this comment

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

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

@natsukium natsukium mentioned this pull request May 3, 2023
Open
@lilyinstarlight lilyinstarlight mentioned this pull request May 3, 2023
Merged
12 tasks
@lilyinstarlight lilyinstarlight mentioned this pull request May 24, 2023
Merged
12 tasks
@lilyinstarlight
Copy link
Member

lilyinstarlight commented Jul 8, 2023

#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 dotlambda force-pushed the bitwarden-cli-source branch from ba22689 to 8d2ea61 Compare July 10, 2023 00:51
@dotlambda
Copy link
Member Author

dotlambda commented Jul 10, 2023

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
lilyinstarlight reviewed Jul 11, 2023
View reviewed changes
@dotlambda dotlambda force-pushed the bitwarden-cli-source branch from 8d2ea61 to f7babb9 Compare July 11, 2023 14:10
@ofborg ofborg bot added 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-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. and removed 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. labels Jul 11, 2023
@ofborg ofborg bot removed the 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. label Jul 11, 2023
@lilyinstarlight
Copy link
Member

lilyinstarlight commented Jul 13, 2023 •
edited
Loading

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
lilyinstarlight reviewed Jul 13, 2023
View reviewed changes
Copy link
Member

@lilyinstarlight lilyinstarlight left a comment

Choose a reason for hiding this comment

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

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!

@dotlambda dotlambda force-pushed the bitwarden-cli-source branch from f7babb9 to 2995a89 Compare July 23, 2023 18:53
lilyinstarlight
lilyinstarlight approved these changes Jul 23, 2023
View reviewed changes
Copy link
Member

@lilyinstarlight lilyinstarlight left a comment

Choose a reason for hiding this comment

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

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...)

@lilyinstarlight lilyinstarlight merged commit 300a19f into NixOS:master Jul 23, 2023
@dotlambda dotlambda deleted the bitwarden-cli-source branch July 23, 2023 23:37
@lilyinstarlight lilyinstarlight mentioned this pull request Jul 23, 2023
Open
@dotlambda
Copy link
Member Author

dotlambda commented Jul 25, 2023 •
edited
Loading

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

@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Jul 25, 2023

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

@dotlambda
Copy link
Member Author

dotlambda commented Jul 25, 2023 •
edited
Loading

remove it from the nodePackage generation

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

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

Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 approved these changes

@Mic92 Mic92 Awaiting requested review from Mic92

@winterqt winterqt Awaiting requested review from winterqt

+1 more reviewer

@lilyinstarlight lilyinstarlight lilyinstarlight approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

6.topic: nodejs Node.js is a free, open-source, cross-platform JavaScript runtime environment 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-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes.

Projects

Repackage node2nix packages
Status: Re-packaged

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dotlambda @SuperSandro2000 @lilyinstarlight