vimPlugins.markdown-preview: init at 2020-09-09 #97781

iclanzan · 2020-09-11T15:37:25Z

Motivation for this change

Added the markdown-preview (neo)vim plugin.

Things done

ceedubs · 2020-09-20T16:48:48Z

Thanks for putting this together, @iclanzan!

I'm not a maintainer of nixpkgs vim plugins, but I am interested in using this plugin, so I gave this a try. I got the error Pre build and node is not found. It looks like this is currently expecting a node executable to be on the path. Should this use a patch or something to change the call to node to use a nix-managed node?

ceedubs · 2020-09-20T17:17:56Z

@iclanzan I tried out the patch approach here and it seems to be working for me. What do you think about incorporating a change like this? I used fruzzy in vim-plugins as an example.

iclanzan · 2020-09-20T17:50:56Z

Nice catch! It was working for me because I have Node installed globally. I wonder if we should specify nodejs in buildInputs instead of patching the source.

ceedubs · 2020-09-20T19:53:48Z

@iclanzan I don't think that adding nodejs to buildInputs will actually help since this is loaded as a vim plugin as opposed to a binary that people launch directly. If you find otherwise then that would be a lot simpler, though.

jonringer · 2020-09-20T22:25:10Z

@iclanzan I don't think that adding nodejs to buildInputs will actually help since this is loaded as a vim plugin as opposed to a binary that people launch directly. If you find otherwise then that would be a lot simpler, though.

This is correct, buildInputs will only make it available during build time, but not during runtime.

You will want to either add nodejs to the propagatedBuildInputs, or patch the source so that finds the nix store path

jonringer · 2020-09-20T22:27:00Z

cc @timokau @evanjs for proper solution

timokau · 2020-09-21T08:44:43Z

Yeah I think propagatedBuildInputs will not work either, since it will not make the node plugin available in the global environment at runtime. You'll have to patch the plugin.

I've wanted some way to automate this for quite a while. We could specify inputs for plugins, and then generate some vimscript to set up PATH etc. But for now, we don't have such a mechanism and I probably won't get around to implementing it.

iclanzan · 2020-09-21T14:46:29Z

I tried propagatedBuildInputs and it does indeed not work. Patching the path to node worked for me too, so I went ahead and committed the changes from @ceedubs.

ceedubs

I can't really speak to the best practices of derivations for Yarn packages, but I can confirm that this approach works for me (Neovim on Mac OS) and that this is a useful plugin. 👍

Edit: it looks like there is an unsuccessful PR check though.

jonringer

I think you forgot to add the lock file

cannot read '/nix/store/mx7qc6dlazvkgmmpji315y6pih7jkmhx-source/app/package.json', since path '/nix/store/j4xpz54q58kk3m4xgw7lbrf24iypa7iz-source.drv' is not valid, at /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/mr-est/packet-spot-eval-3/lib/trivial.nix:282:24

Also, do you mind squishing all the node commits into a single one:

vimPlugins.markdown-preview: enable node support

iclanzan · 2020-09-21T17:36:06Z

My understanding is that both package.json and yarn.lock are read from the source by default:

nixpkgs/pkgs/development/tools/yarn2nix-moretea/yarn2nix/default.nix

Lines 226 to 227 in 75ee187

    
           packageJSON ? src + "/package.json", 
        
           yarnLock ? src + "/yarn.lock",

Locally I have not had to add any of them as argument. Can you help me understand why I need to manually add them here? And do I need to add them both?

do you mind squishing all the node commits into a single one

Will do 👍

jonringer · 2020-09-21T18:04:03Z

Locally I have not had to add any of them as argument. Can you help me understand why I need to manually add them here? And do I need to add them both?

I'm not entirely sure either (little experience with node packaging), but it looks like it expects for the file to be available locally

iclanzan · 2020-09-21T21:04:12Z

I have committed both the package.json and yarn.lock files, but now ofborg is complaining about yarnNix, which should be autogenerated 🤷‍♂️

nixpkgs/pkgs/development/tools/yarn2nix-moretea/yarn2nix/default.nix

Lines 389 to 391 in 75ee187

    
           # yarn2nix is the only package that requires the yarnNix option. 
        
           # All the other projects can auto-generate that file. 
        
           yarnNix = ./yarn.nix;

iclanzan · 2020-09-21T21:10:17Z

I see that other projects using mkYarnPackage have all 3 files committed so I gather I am not the first to run into this issue. For example codimd:
https://github.com/NixOS/nixpkgs/tree/75ee18766ac52a97df1dc55a11a2d44cb0e68d0e/pkgs/servers/web-apps/codimd

So I guess I have to commit the generated yarn.nix file too. One question I have is where to put these files. Should I create a new folder in pkgs/misc/vim-plugins for plugins that are manually created and have extra files? Maybe pkgs/misc/vim-plugins/plugins/markdown-preview/?

ceedubs · 2020-09-22T02:56:50Z

pkgs/misc/vim-plugins/overrides.nix

+      src = "${src}/app";
+      packageJSON = ./plugins/markdown-preview/package.json;
+      yarnLock = ./plugins/markdown-preview/yarn.lock;
+      yarnNix = ./plugins/markdown-preview/yarn.nix;


I don't think that you need to add separate package.json and yarn.lock files to this repo. They are both included in the markdown-preview.nvim repo. I think that you just need to do something like:

packageJSON = src + "/package.json"; yarnLock = src + "/package.json";

And since the yarn.lock should ensure stable results, you shouldn't need to check in a yarn.nix with this PR; yarn2nix should be able to generate one itself.

At least this is my understanding. I could definitely be wrong.

Unfortunately the ofborg checks in this PR fail when any of the 3 files are not local to the nixpkgs repo.

I thought that the original problem was that it was trying to look in app/package.json because of its default relative to the src that was defined as src + "/app". Because of the squashing it's tough for me to see the history of what failed. Was there a commit where you fixed that but it still complained that the files weren't local?

Yes, I had incrementally added those files and watched ofborg fail. At @jonringer’s recommendation I squashed the commits into 1 so all that history is lost.

In reality all 3 files can be omitted and the plugin builds fine. It’s only these ofborg tests that fail. If you look at other usages of mkYarnPackage in nixpkgs you will notice that they have these 3 files commited:
https://github.com/NixOS/nixpkgs/tree/75ee18766ac52a97df1dc55a11a2d44cb0e68d0e/pkgs/servers/web-apps/codimd

This is mostly likely due to "Import from derivation" or IFD. Ofborg doesn't like you creating one derivation, then re-using outputs of that derivation to create another. Which is why you need to check-in the deps file, which eliminates one of the links in the import chain.

@oxalica you've done some node work.

Why can't we put this under nodePackages?

oxalica · 2020-09-23T12:48:25Z

There's an discussion in #83499 about making a shared yarn.nix just like our global node-packages.nix.
Another approach is that making mkYarnPackage accept an output hash for dependency vendor (like buildRustPackage).

Anyway, I'm not in favor of importing from derivation. But if so, we currently need to keep yarn.{lock,nix} in tree, though they are both large in size. 😟

stelcodes · 2021-05-17T14:55:24Z

I would love for this plugin to get added to nixpkgs. Any way I can help?

ceedubs · 2021-05-17T16:21:23Z

In case it's helpful to anyone else, I noticed that the nixpkgs unstable branch now includes vim-markdown-composer which seems to be a good replacement for markdown-preview written in Rust.

iclanzan requested a review from jonringer as a code owner September 11, 2020 15:37

iclanzan mentioned this pull request Sep 11, 2020

SyntaxError: Unexpected token iamcco/markdown-preview.nvim#168

Closed

ofborg bot added the 6.topic: vim label Sep 11, 2020

ceedubs approved these changes Sep 21, 2020

View reviewed changes

jonringer reviewed Sep 21, 2020

View reviewed changes

iclanzan force-pushed the master branch 2 times, most recently from ddf4c81 to a2911bd Compare September 21, 2020 20:13

iclanzan force-pushed the master branch from a2911bd to 81d9b61 Compare September 21, 2020 21:28

ofborg bot added 8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Sep 21, 2020

ceedubs suggested changes Sep 22, 2020

View reviewed changes

SuperSandro2000 added the 2.status: merge conflict label Mar 21, 2021

iclanzan closed this Sep 23, 2021

iclanzan force-pushed the master branch from 81d9b61 to ed4f741 Compare September 23, 2021 00:00

github-actions bot removed the 6.topic: vim label Sep 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vimPlugins.markdown-preview: init at 2020-09-09 #97781

vimPlugins.markdown-preview: init at 2020-09-09 #97781

iclanzan commented Sep 11, 2020

ceedubs commented Sep 20, 2020

ceedubs commented Sep 20, 2020

iclanzan commented Sep 20, 2020

ceedubs commented Sep 20, 2020

jonringer commented Sep 20, 2020

jonringer commented Sep 20, 2020 •

edited

Loading

timokau commented Sep 21, 2020

iclanzan commented Sep 21, 2020

ceedubs left a comment •

edited

Loading

jonringer left a comment

iclanzan commented Sep 21, 2020

jonringer commented Sep 21, 2020

iclanzan commented Sep 21, 2020

iclanzan commented Sep 21, 2020

ceedubs Sep 22, 2020

iclanzan Sep 22, 2020

ceedubs Sep 22, 2020

iclanzan Sep 22, 2020

jonringer Sep 22, 2020

jonringer Sep 22, 2020

SuperSandro2000 May 17, 2021

oxalica commented Sep 23, 2020

stelcodes commented May 17, 2021

ceedubs commented May 17, 2021

vimPlugins.markdown-preview: init at 2020-09-09 #97781

vimPlugins.markdown-preview: init at 2020-09-09 #97781

Conversation

iclanzan commented Sep 11, 2020

Motivation for this change

Things done

ceedubs commented Sep 20, 2020

ceedubs commented Sep 20, 2020

iclanzan commented Sep 20, 2020

ceedubs commented Sep 20, 2020

jonringer commented Sep 20, 2020

jonringer commented Sep 20, 2020 • edited Loading

timokau commented Sep 21, 2020

iclanzan commented Sep 21, 2020

ceedubs left a comment • edited Loading

Choose a reason for hiding this comment

jonringer left a comment

Choose a reason for hiding this comment

iclanzan commented Sep 21, 2020

jonringer commented Sep 21, 2020

iclanzan commented Sep 21, 2020

iclanzan commented Sep 21, 2020

ceedubs Sep 22, 2020

Choose a reason for hiding this comment

iclanzan Sep 22, 2020

Choose a reason for hiding this comment

ceedubs Sep 22, 2020

Choose a reason for hiding this comment

iclanzan Sep 22, 2020

Choose a reason for hiding this comment

jonringer Sep 22, 2020

Choose a reason for hiding this comment

jonringer Sep 22, 2020

Choose a reason for hiding this comment

SuperSandro2000 May 17, 2021

Choose a reason for hiding this comment

oxalica commented Sep 23, 2020

stelcodes commented May 17, 2021

ceedubs commented May 17, 2021

jonringer commented Sep 20, 2020 •

edited

Loading

ceedubs left a comment •

edited

Loading