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

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

Closed
wants to merge 0 commits into from

Conversation

iclanzan
Copy link
Contributor

Motivation for this change

Added the markdown-preview (neo)vim plugin.

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 CONTRIBUTING.md.

@ceedubs
Copy link
Contributor

ceedubs commented Sep 20, 2020

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
Copy link
Contributor

ceedubs commented Sep 20, 2020

@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
Copy link
Contributor Author

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
Copy link
Contributor

ceedubs commented Sep 20, 2020

@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
Copy link
Contributor

@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
Copy link
Contributor

jonringer commented Sep 20, 2020

cc @timokau @evanjs for proper solution

@timokau
Copy link
Member

timokau commented Sep 21, 2020

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
Copy link
Contributor Author

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.

Copy link
Contributor

@ceedubs ceedubs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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

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
Copy link
Contributor

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
Copy link
Contributor Author

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

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

@iclanzan
Copy link
Contributor Author

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/?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oxalica you've done some node work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't we put this under nodePackages?

@oxalica
Copy link
Contributor

oxalica commented Sep 23, 2020

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
Copy link
Contributor

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

@ceedubs
Copy link
Contributor

ceedubs commented May 17, 2021

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 iclanzan closed this Sep 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants