-
-
Notifications
You must be signed in to change notification settings - Fork 14k
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
add nodePackages vega-cli and vega-lite #97908
Conversation
I tried to run
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
@MMesch It's almost impossible to merge changes to node packages, unfortunately. Could you please fix them and ping me when you are ready? Preferably, Once that'll be done, I'd be happy to get your help on #96961 . |
Hi @doronbehar
What do you mean with this? Is it possible or not? I looked on your PR and saw that you suggested generating nix expressions for the packages you want to add independently. I have done that equally for vega-cli and vega-lite on my machine. Do you think it would be better to move forward like this? Also, I already added |
I mean that as a contributor, with no merge permissions, it's impossible to put out a PR, and when the time comes for a committer to merge it, the PR has no merge conflicts.
It depends on you. For balena-cli, I realized at a certain point I have to do it because their releases are too frequent for node packages' |
I don't think so, but I haven't checked. |
@doronbehar I have rebased and regenerated node-packages.nix for the two relevant commits. Maybe I didn't understand if you wanted to do anything else. Let me know! In case there are conflicts again, this can be rebased from the nodePackages folder with:
(takes ~2h on my machine) |
Right! Sorry. It should be deleted. Do you want me to do it again?
Doron Behar <notifications@github.com> schrieb am Sa., 19. Sep. 2020, 13:41:
… ***@***.**** commented on this pull request.
------------------------------
In pkgs/development/node-packages/default.bak
<#97908 (comment)>:
> @@ -0,0 +1,200 @@
+{ pkgs, nodejs, stdenv }:
This file is a Git rebase leftover right?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#97908 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAT2HK2QY5ZJI4UQYK3UGZ3SGSKGVANCNFSM4RKVLFJQ>
.
|
You should be able to spare yourself running |
I hope it's ok now @doronbehar . I ran the |
773aa67
to
ae9af63
Compare
Sorry for the formatting issues. How about now @doronbehar ? |
substituteInPlace $out/lib/node_modules/vega-lite/bin/vl2pdf \ | ||
--replace "npx -p vega vg2pdf" "${self.vega-cli}/bin/vg2pdf" | ||
substituteInPlace $out/lib/node_modules/vega-lite/bin/vl2svg \ | ||
--replace "npx -p vega vg2svg" "${self.vega-cli}/bin/vg2svg" | ||
substituteInPlace $out/lib/node_modules/vega-lite/bin/vl2png \ | ||
--replace "npx -p vega vg2png" "${self.vega-cli}/bin/vg2png" | ||
''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
substituteInPlace $out/lib/node_modules/vega-lite/bin/vl2pdf \ | |
--replace "npx -p vega vg2pdf" "${self.vega-cli}/bin/vg2pdf" | |
substituteInPlace $out/lib/node_modules/vega-lite/bin/vl2svg \ | |
--replace "npx -p vega vg2svg" "${self.vega-cli}/bin/vg2svg" | |
substituteInPlace $out/lib/node_modules/vega-lite/bin/vl2png \ | |
--replace "npx -p vega vg2png" "${self.vega-cli}/bin/vg2png" | |
''; | |
substituteInPlace $out/lib/node_modules/vega-lite/bin/vl2pdf \ | |
--replace "npx -p vega vg2pdf" "${self.vega-cli}/bin/vg2pdf" | |
substituteInPlace $out/lib/node_modules/vega-lite/bin/vl2svg \ | |
--replace "npx -p vega vg2svg" "${self.vega-cli}/bin/vg2svg" | |
substituteInPlace $out/lib/node_modules/vega-lite/bin/vl2png \ | |
--replace "npx -p vega vg2png" "${self.vega-cli}/bin/vg2png" | |
''; |
Sorry for the nitpick, but it should be perfect now!
@MMesch after fixing once more the indentation, please apply this patch: diff --git i/pkgs/development/node-packages/default.nix w/pkgs/development/node-packages/default.nix
index 71b9bd77411..8d8d9fc89ae 100644
--- i/pkgs/development/node-packages/default.nix
+++ w/pkgs/development/node-packages/default.nix
@@ -181,6 +181,8 @@ let
# https://sharp.pixelplumbing.com/install
vips
+ libsecret
+ self.node-gyp-build
self.node-pre-gyp
];
}; And commit it as: It's not related to your PR, but these are new deps that your update to all of node packages require - since you ran |
Ah s... I forgot that one. Unfortunately I can't do it until tomorrow. If
you have the time to amend this and merge go ahead, otherwise it has to
wait.
Thabks for the help and sorry for all the work :( I tried to use nixfmt on
this but it changes a lot of other things.
Doron Behar <notifications@github.com> schrieb am Sa., 19. Sep. 2020, 16:34:
… ***@***.**** commented on this pull request.
------------------------------
In pkgs/development/node-packages/default.nix
<#97908 (comment)>:
> + substituteInPlace $out/lib/node_modules/vega-lite/bin/vl2pdf \
+ --replace "npx -p vega vg2pdf" "${self.vega-cli}/bin/vg2pdf"
+ substituteInPlace $out/lib/node_modules/vega-lite/bin/vl2svg \
+ --replace "npx -p vega vg2svg" "${self.vega-cli}/bin/vg2svg"
+ substituteInPlace $out/lib/node_modules/vega-lite/bin/vl2png \
+ --replace "npx -p vega vg2png" "${self.vega-cli}/bin/vg2png"
+ '';
⬇️ Suggested change
- substituteInPlace $out/lib/node_modules/vega-lite/bin/vl2pdf \
- --replace "npx -p vega vg2pdf" "${self.vega-cli}/bin/vg2pdf"
- substituteInPlace $out/lib/node_modules/vega-lite/bin/vl2svg \
- --replace "npx -p vega vg2svg" "${self.vega-cli}/bin/vg2svg"
- substituteInPlace $out/lib/node_modules/vega-lite/bin/vl2png \
- --replace "npx -p vega vg2png" "${self.vega-cli}/bin/vg2png"
- '';
+ substituteInPlace $out/lib/node_modules/vega-lite/bin/vl2pdf \
+ --replace "npx -p vega vg2pdf" "${self.vega-cli}/bin/vg2pdf"
+ substituteInPlace $out/lib/node_modules/vega-lite/bin/vl2svg \
+ --replace "npx -p vega vg2svg" "${self.vega-cli}/bin/vg2svg"
+ substituteInPlace $out/lib/node_modules/vega-lite/bin/vl2png \
+ --replace "npx -p vega vg2png" "${self.vega-cli}/bin/vg2png"
+ '';
Sorry for the nitpick, but it should be perfect now!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#97908 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAT2HK7AGDYY33KHZW4DWL3SGS6RDANCNFSM4RKVLFJQ>
.
|
Co-authored-by: Doron Behar <doron.behar@gmail.com>
ae9af63
to
90ef425
Compare
90ef425
to
9c12892
Compare
Many thanks for you help @doronbehar . How can I help with your PR? |
TBH I only want someone to approve it for the record, and verify that it works. And comments upon meta data or such would be welcome #96961 . |
Motivation for this change
Adds the vega and the vega-lite command line tools for plotting. This means the binaries:
Vegalite required substituting some lines that call
npx
to installvega
on the fly. Maybe there is a better solution. I tested these tools with:Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)