-
-
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
nodePackages.mermaid-cli: init 8.4.8 #87766
Conversation
Thanks @ysndr! Is there a way I can test this branch as a channel? Or do I have to pull down a local |
Yes, using the The package name is nodePackages.mermaid I'm on mobile right now so I can not tell exactly, tab completion is your friend though ;) |
Huh, I see |
Yes as I said in the description, the path is quite ugly actually, I'll look into it again if there is probably a better way.. |
Yeah, quoting it worked, good idea. I got a weird break:
|
What kind of system are you using? Are you on NixOS or nix on linux? It's probably related to puppeteer being download during the build phase which is not possible in the sandbox. Maybe this is less strict on MacOS where I used it. |
@ysndr Yeah I'm on NixOS, this is weird. |
Yes, while installing and building the builder does not have access to the internet (and other niceties) to ensure pureness. |
Progress! Here's what's up now.
|
@matthew-piziak can you try it now? I missed including |
Notice: I added it as |
It works! Thank you! I don't suppose you can tell me how to add custom configuration files to the derivation options? Here's what I have:
In case you're curious, here's my intention. It's to support generation of nice high-res PNGs on the command line without a browser: mermaidjs/mermaid.cli#3 (comment) |
With some help from IRC I have this:
This setup works great! Thank you. :) |
Glad to hear that!
You should not need @svanderburg are we good to merge this? |
Please rework your PR. It now has a merge conflict after PR #89184 has been merged |
Did the rework. It still works for me on MacOS at least. |
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.
Please change commit message to: nodePackages.mermaid-cli: init 8.4.8
Looks like there are merge conflicts again.. any chance at a rebase and merge? |
Sure, all good things are three. |
Since May I've been building directly from the pull request with |
0f00d8b
to
c1834f4
Compare
Let's try again then. |
@matthew-piziak you want to try if everything works with it? |
fwiw |
Thank you once again @ysndr. Looks good to me! graph TD
A[Mermaid PR] --> B{{Merge Conflict?}};
B -->|Yes| C[Resolve];
C --> B;
B --> |No| E{{Wait for Merge}};
E --> |Merged| S[Success!]:::success;
classDef success fill:#3e3;
E --> |Too Slow| B;
|
Who is responsible for merging this though? I have no rights here :D |
@ysndr That's the million-dollar question. I've pinged it on #nixos about four times now. The contributing guidelines are silent on the topic of actually getting PRs merged. |
Well, it's pretty natural people are feeling unsure about something that, in addition to stated goal, also updates a ton of nodePackages… But maybe npm cannot be made worse… |
Sorry about the poor contribution experience! We have a discourse thread where PRs in dire need of committer attention can be mentioned: https://discourse.nixos.org/t/prs-in-distress/3604 |
@7c6f434c is it even possible to add an npm package without changing (updating) a ton of other libraries at the same time? |
Thank you @7c6f434c @lheckemann!! It's wonderful to have this hit master. |
@ysndr I guess updating could be avoided with heroic effort, but you did nothing wrong — you just got caught in a particularly annoying process gotcha. |
Weird, I can't access |
https://status.nixos.org/ it only updated 36 minutes ago after having build problems for a while, did you try less than 36 minutes ago? |
@lheckemann Tried it again just now. I still get |
I also made sure to set |
|
Weird. If I |
Same for |
Ah, it's because |
Motivation for this change
I needed mermaid and thought I could also share this.
I am not sure whether the
@mermaid-js
part is necessary, it does produce some ugly store paths.Things done
node-packges*.nix
using thegenerate.sh
scriptsandbox
innix.conf
on non-NixOS linux)Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)