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

nodePackages: init sass clubhouse-cli makam inliner undollar (combine several PRs) #100606

merged 1 commit into from Oct 16, 2020


Copy link

@cideM cideM commented Oct 15, 2020

This PR is my attempt to help with the issue (and potential, temporary solution) described in this Discourse thread.

Every time someone changes the NodeJS package list and generates a new .json file, it results in merge conflicts for subsequent NodeJS package list modifications, since they all affect the same file.

What I did was manually go through some PRs, copy their changes but skipping the .json file changes and then I just ran once. I included all authors through co-authoring.

These are the PRs that I combined:

I haven't done any testing yet since I'm kinda still at work so I'd go through the checklist later, in case this approach here is considered worth doing.

I think this approach is easy to automate. Just go through a list of PRs, get their diffs, apply them but skipping the .json changes, accumulate the commit authors while doing so and then generate the .json files once.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits
Copy link

@nixos-discourse nixos-discourse commented Oct 15, 2020

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

@ofborg ofborg bot requested a review from midchildan Oct 15, 2020
@cideM cideM force-pushed the node-merge-pr branch from ba11a6d to 014bb45 Oct 15, 2020
@cideM cideM changed the title Merge node-packages PRs to add: sass clubhouse-cli makam inliner undollar nodePackages: init sass clubhouse-cli makam inliner undollar (combine several PRs) Oct 15, 2020
Copy link

@ryantm ryantm commented Oct 15, 2020

Result of nixpkgs-review pr 100606 1

2 packages marked as broken and skipped:
- rambox-pro
- teleprompter
54 packages built:
- antora
- bitwarden-cli
- castnow
- create-cycle-app
- emojione
- epgstation
- etcher
- fast-cli
- gtop
- image_optim
- imapnotify
- iosevka
- joplin
- lessc
- lumo
- mastodon-bot
- mirakurun
- netlify-cli
- parity-ui
- pm2
- postcss-cli
- pulp
- python37Packages.batchspawner
- python37Packages.dockerspawner
- python37Packages.jupyterhub
- python37Packages.jupyterhub-ldapauthenticator
- python37Packages.jupyterhub-systemdspawner
- python37Packages.jupyterhub-tmpauthenticator
- python37Packages.oauthenticator
- python38Packages.batchspawner
- python38Packages.dockerspawner
- python38Packages.jupyterhub
- python38Packages.jupyterhub-ldapauthenticator
- python38Packages.jupyterhub-systemdspawner
- python38Packages.jupyterhub-tmpauthenticator
- python38Packages.oauthenticator
- redoc-cli
- ripcord
- shout
- slack
- styx
- thelounge
- triton
- twemoji-color-font
- vimPlugins.coc-eslint
- vimPlugins.coc-go
- vimPlugins.coc-metals
- vimPlugins.coc-prettier
- vimPlugins.coc-stylelint
- vimPlugins.coc-tslint
- vimPlugins.coc-vetur
- vimPlugins.coc-yaml
- wasm-text-gen
- yaml-language-server

Copy link

@ryantm ryantm commented Oct 15, 2020

I tried bitwarden-cli and lessc binaries, and the both display help messages.

Copy link

@ryantm ryantm commented Oct 15, 2020

@cideM Any reason you see not to merge now?

Copy link
Contributor Author

@cideM cideM commented Oct 15, 2020

I tried building all packages on my Macbook and makam fails with `attribute 'glibc' is missing. Comes from

    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/ "$out/lib/node_modules/makam/makam-bin-linux64"

I'll have to look into the proper Darwin replacement for glibc.

A quick glance at how all of this works tells me that I can't just replace glibc with some Darwin replacement I guess, since the ld-linux.. obviously won't exist. Maybe someone else knows how this is typically handled? I'm pretty clueless when it comes to shared object files :(

Other options:

Will continue tomorrow

This commit combines several individual PRs which would have resulted in
merge conflicts in the generated JSON files. Instead, the ""
script is only ran once.

Due to "makam" not building on MacOS in the form it was originally added
in the PR I made some adjustments to this diff.

List of added packages:

- nodePackages.clubhouse-cli: init at 2.1.0
- nodePackages.makam: init at 0.7.17
- nodePackages.inliner: init at 1.13.1
- nodePackages.sass: init at 1.27.0
- nodePackages.undollar: init at 1.0.0

Co-authoring is used to preserve contributions.

Co-authored-by: Changlin Li <>
Co-authored-by: Pasquale <>
Co-authored-by: Teodoro Freund <>
Co-authored-by: Tobias Mayer <>
Co-authored-by: vladki <>
@cideM cideM force-pushed the node-merge-pr branch from 014bb45 to b1b63b6 Oct 16, 2020
Copy link
Contributor Author

@cideM cideM commented Oct 16, 2020

I made a change to the makam package. I simply wrapped the patchelf call in a condition so it only runs when we're on linux. The reason for this is that otool -L ./result/lib/node_modules/makam/makam-bin-darwin64 only turns up libSystem. I know more or less nothing about how dynamic linking works on MacOS but I guess the patchelf call here is really only necessary on Linux.

I can run makam so it outputs help text. So from my side this is now good to merge @ryantm

Also I didn't re-run after adding the if because that would only update NodeJS dependencies and the change to the patchelf call shouldn't affect that, right?

@ryantm ryantm merged commit a1a01bd into NixOS:master Oct 16, 2020
17 checks passed
@ryantm ryantm mentioned this pull request Oct 16, 2020
10 tasks
@ryantm ryantm mentioned this pull request Oct 16, 2020
7 tasks
@ryantm ryantm mentioned this pull request Oct 16, 2020
10 tasks
@doronbehar doronbehar mentioned this pull request Dec 5, 2020
10 tasks
@cideM cideM deleted the node-merge-pr branch Apr 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants