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

pnpm.fetchDeps: multiple workspaces; better pnpmInstallFlags #350751

Merged
merged 4 commits into from
Oct 24, 2024

Conversation

pyrox0
Copy link
Member

@pyrox0 pyrox0 commented Oct 23, 2024

Adds support for installing multiple PNPM workspace packages' dependencies with the pnpmWorkspaces variable, and adds pnpmInstallFlags to other parts of PNPM configuration. Also adds appropriate documentation.

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

@pyrox0 pyrox0 changed the title pnpm.fetchDeps: multiple workspaces better pnpmInstallFlags pnpm.fetchDeps: multiple workspaces; better pnpmInstallFlags Oct 23, 2024
@pyrox0 pyrox0 force-pushed the pnpm/installflags-and-multi-workspace branch from ac9bcc3 to 3981d80 Compare October 23, 2024 19:11
@pyrox0
Copy link
Member Author

pyrox0 commented Oct 23, 2024

Forgot to push the commits with the related package changes. I expect a good ofborg eval, can't check locally atm sorry.

Copy link
Contributor

@gepbird gepbird left a comment

Choose a reason for hiding this comment

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

Ofborg failed because you forgot to change pnpmWorkspace in pnpmDeps.

Also would you mind splitting up your first commit? It contains many formatting changes that makes the diff a bit hard to read.

Edit: eval fails for some other reason too

pkgs/by-name/ba/bash-language-server/package.nix Outdated Show resolved Hide resolved
@gepbird
Copy link
Contributor

gepbird commented Oct 23, 2024

Aside from the those and some minor formatting issues that we don't have time for, this LGTM! Tested the 2 affected packages on x86_64-linux. I'm going to sleep and may not be available before this should be merged, I'd appreciate if someone could run a nixpkgs-review on this.

@pyrox0
Copy link
Member Author

pyrox0 commented Oct 23, 2024

Ofborg failed because you forgot to change pnpmWorkspace in pnpmDeps.

Also would you mind splitting up your first commit? It contains many formatting changes that makes the diff a bit hard to read.

Edit: eval fails for some other reason too

The formatting changes come from the throwIf call being added, should I do a commit with just that and then a reformat commit after?

@emilazy
Copy link
Member

emilazy commented Oct 23, 2024

My wisdom is to do assert lib.throwIf … true;, which formats nicely and is a somewhat pleasing “statement”. Also good for asserts.

@pyrox0 pyrox0 force-pushed the pnpm/installflags-and-multi-workspace branch from c201b91 to 9ac2535 Compare October 24, 2024 05:08
@pyrox0
Copy link
Member Author

pyrox0 commented Oct 24, 2024

My wisdom is to do assert lib.throwIf … true;, which formats nicely and is a somewhat pleasing “statement”. Also good for asserts.

Fixed, a sanity check would be nice though just to double check the code actually makes sense. Evals properly on my machine, though.

Copy link
Contributor

@gepbird gepbird left a comment

Choose a reason for hiding this comment

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

Thanks!

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

1 package built:
  • nixpkgs-manual

@gepbird
Copy link
Contributor

gepbird commented Oct 24, 2024

Sorry for the early approval, packages fail for me even though ofborg and nixpkgs-review passed.
For example when trying to build astro-language-server the assert triggers.

Copy link
Contributor

@gepbird gepbird left a comment

Choose a reason for hiding this comment

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

If we are talking about formatting here are my nits I held back due to the time pressure. We have ~10 hours to merge this with the pnpmWorkspace deprecation.

doc/languages-frameworks/javascript.section.md Outdated Show resolved Hide resolved
doc/languages-frameworks/javascript.section.md Outdated Show resolved Hide resolved
@Scrumplex
Copy link
Member

If we are talking about formatting here are my nits I held back due to the time pressure

I am sure we will get this merged by then. There are three committers involved/pinged in this thread ^^

@emilazy
Copy link
Member

emilazy commented Oct 24, 2024

More like 13¾ hours, for what it’s worth. I agree that we should be okay for time (sorry that I can’t review properly myself, I have my own PRs to rush in before the freeze).

@pyrox0 pyrox0 force-pushed the pnpm/installflags-and-multi-workspace branch from 9ac2535 to 04792da Compare October 24, 2024 14:10
@pyrox0
Copy link
Member Author

pyrox0 commented Oct 24, 2024

Fixed all noted issues.

Copy link
Member

@Scrumplex Scrumplex left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@gepbird gepbird left a comment

Choose a reason for hiding this comment

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

Changes LGTM! astro-language-server, bash-language-server and another pnpm package like heroic builds.

I started nixpkgs-review (on a different machine) and I'm getting 52 rebuilds compared to the previous 1 rebuild.

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 good overall, small comments left. Haven't read other comments in the thread.

@gepbird
Copy link
Contributor

gepbird commented Oct 24, 2024

nixpkgs-review looks good, I won't wait out the electron build.

2024-10-24_20-07

@pyrox0 pyrox0 force-pushed the pnpm/installflags-and-multi-workspace branch from 04792da to 42fb646 Compare October 24, 2024 19:37
Copy link
Contributor

@gepbird gepbird left a comment

Choose a reason for hiding this comment

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

Throws an eval time error when pnpmWorkspace is both in a package derivation and in pnpm.fetchDeps, and a build time error when pnpmWorkspace is only in the derivation, as expected. Those 3 packages build, LGTM.

@Scrumplex Scrumplex merged commit 98c6c17 into NixOS:master Oct 24, 2024
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants