@ryantm I would be happy to help as well. I use nodejs + nix at work on daily basis and should be able to commit around 2 hours a week to this.
1 Like
@jonringer @ryantm Cool, I’ve added myself in codeowners: add cprussin as a codeowner for nodejs & deno by cprussin · Pull Request #99666 · NixOS/nixpkgs · GitHub.
Few questions:
-
Is the expectation here purely to help review PRs or are there larger plans for improving the nodejs ecosystem in nixpkgs that maintainers should be thinking about? What level of freedom & discretion about those changes do maintainers have and where is the appropriate place to discuss significant changes before making them?
-
Are there discussion forums I should be joining as a maintainer? I sit on the #nix channel on the functionalprogramming slack, but I don’t notice many maintainers in that channel apart from @cdepillabout. I’m happy to become more active elsewhere too.
1 Like
oh and @adrian-gierakowski and @cideM I’d be happy to add you two as CODEOWNERS in the PR I opened, just let me know if ya’ll are interested still.
Short term, just PRs, which is the intent of this discussion thread. Changes to the node ecosystem should probably done through an issue and bring in previous contributors to make sure there’s a marked improvement.
Remember that people will be “consuming” the changes, and that nixpkgs can be thought of as an “api”. Breaking changes should be few, and warranted
Discourse is the only official forum/mailing list. IRC is the only official chat platform. Generally most nixpkgs affecting discussions happen on github. The other chat channels are usually there for brain storming, collaboration, or help. You’re not required participate in the other chat circles. (E.g. I spend the most time in discord)
1 Like
Thanks a bunch, that’s helpful context.
FWIW when I was talking about “significant changes” I mostly had in mind internal plumbing or additions, not breaking changes. No question that breaking changes should be few and far between and well justified. But in terms of anything significant, it seems a github issue is the appropriate starting point.
1 Like
Moving the discussion back here as I’ve closed the PR since it seems github doesn’t support CODEOWNERS who don’t have write access to the repository.
Any suggestions on how to effectively contribute as a maintainer without being listed as a CODEOWNER?
2 Likes
Before we get some tools made, you could do these things:
- Ask people who made PRs to remove their changes to node-packages.nix.
- Consolidate the changes from the PRs yourself with no changes to node-packages.nix.
- Make separate PRs that only update node-packages.nix via the generate script.
You can ping me whenever you need something merged.
2 Likes
the node infrastructure heavily uses node2nix, so the node2nix issue tracker might also be a good place to discuss some things.
Personally, for me (as someone who is not really too familiar with node, but ended up maintaining one node package (mastodon-bot)) the main pain point is that when you use node2nix to update the dependencies for one package, it also updates the dependencies for all other packages.
I think that is a big cause of the many conflicts between PR’s updating node packages, and also makes reviewing harder: I can easily test the one package I maintain still works, but am reluctant to approve a PR that also updates the dependencies for all those other node packages.
See also this node2nix issue.
Sorry I was away from home on a communion and didn’t have time to check in. I’m still up for the task! I see you already said that apparently this CODEOWNER thing doesn’t work without write permissions so I think I’ll just start by trying to make some time for taking a look at existing node PRs and doing the steps that @ryantm outlined.
5 Likes
@ryantm Is there an easy way for you to massively label every PR that touches node-packages.nix ?
Could serve as a quick work around:
Edit: at the very least the 6:topic label doesn’t seem to work. I checked in one of my node submissions.
2 Likes
Also, as a start, the documentation should be updated to tell contributors to not run ./generate.sh:
They should probably run it, so they can test their work, but then not commit it.
So I went through the process of merging three PRs and I have a few questions because I don’t want to step on anyone’s toes. Right now I just manually cherry-picked the changes from each fork/PR and then rewrote the author to preserve the information who put in the work to add something new. I then want to run the generate.sh for each commit. It’s now running as I’m writing this and I have no idea if this will take hours or days on my measly AMD Ryzen 7 1700.
The pragmatic alternative would be to just take the changes from all PRs, add them as a single commit, run generate.sh once, and maybe refer to all authors in the commit message? But I don’t know GitHub well enough to determine if either of these two options will properly preserve the commit in e.g., people’s dashboards and just in case someone really cares about having their commits to Nixpkgs properly reflected there, I thought I’d ask first.
I would have created a PR so you can see the changes but like I said generate is still running. The gist is (without the generated stuff)
nixpkgs-master (master↑3|✔) $ git shortlog --max-count=3
Changlin Li (1):
nodePackages.inliner: init at 1.13.1
Teodoro Freund (1):
Added the makam package to the nodePackages
Tobias Mayer (1):
nodePackages.clubhouse-cli: init at 2.1.0
diff --git a/pkgs/development/node-packages/default.nix b/pkgs/development/node-packages/default.nix
index 4ef3de0bb4e..c86bc964fd6 100644
--- a/pkgs/development/node-packages/default.nix
+++ b/pkgs/development/node-packages/default.nix
@@ -73,6 +73,14 @@ let
nativeBuildInputs = drv.nativeBuildInputs or [] ++ [ pkgs.psc-package self.pulp ];
});
+ makam = super.makam.override {
+ buildInputs = [ pkgs.nodejs pkgs.makeWrapper ];
+ postFixup = ''
+ wrapProgram "$out/bin/makam" --prefix PATH : ${stdenv.lib.makeBinPath [ pkgs.nodejs ]}
+ patchelf --set-interpreter ${stdenv.glibc}/lib/ld-linux-x86-64.so.2 "$out/lib/node_modules/makam/makam-bin-linux64"
+ '';
+ };
+
mirakurun = super.mirakurun.override rec {
nativeBuildInputs = with pkgs; [ makeWrapper ];
postInstall = let
diff --git a/pkgs/development/node-packages/node-packages.json b/pkgs/development/node-packages/node-packages.json
index 77b398caeec..8cd631cf070 100644
--- a/pkgs/development/node-packages/node-packages.json
+++ b/pkgs/development/node-packages/node-packages.json
@@ -18,6 +18,7 @@
, "browserify"
, "castnow"
, "clean-css-cli"
+, "clubhouse-cli"
, "coc-css"
, "coc-emmet"
, "coc-eslint"
@@ -82,6 +83,7 @@
, "gitmoji-cli"
, "graphql-cli"
, "grunt-cli"
+, "makam"
, "gtop"
, "gulp"
, "gulp-cli"
@@ -90,6 +92,7 @@
, "htmlhint"
, "http-server"
, "hueadm"
+, "inliner"
, "imapnotify"
, "indium"
, "insect"
TL;DR: I really want to help I just want to make sure I do it in the right way before automating anything.
1 Like
Thank you for taking this on! I’m sure we can figure out something if we work on it.
Why after each commit? I think cherry-picking a string of commits with a generate commit at the end would work.
I have no idea if this will take hours or days on my measly AMD Ryzen 7 1700.
I remember it running pretty quickly for me. Hope it doesn’t take that long!
The idea was that running generate.sh after each commit would preserve a working git history whereas without it you might look at a commit and determine it includes node package x but because generate.sh was never run it actually doesn’t
So the PR is now merged I think my next step will be to turn this into a bash script so the process is automated.
4 Likes
@cideM I’d like to get a node package in the regular nodePackages sets and not in a separate default.nix - i.e I’d like to supersede this:
https://github.com/NixOS/nixpkgs/pull/96961
In favor of you cherry-picking this commit into your next round of nodePackages additions.
I’m working on an update to iosevka, whose package.json is linked in node-packages.json so that its dependencies are available for use at build time.
Do batching updates and not running node-packages/generate.sh apply in this case as well? If so, how does that work with automatic updates by r-ryantm (as updates will fail to build if the dependencies change)?
Sorry I’ve been busy with some other stuff for the past two weeks but I’ll dedicate some time to the automation this weekend and do another merge PR
1 Like