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

bug: If a plugin having dev = true and other plugins depends on the plugin, dev = true is ignored #982

Closed
3 tasks done
kyoh86 opened this issue Aug 8, 2023 · 18 comments
Closed
3 tasks done
Labels
bug Something isn't working

Comments

@kyoh86
Copy link
Contributor

kyoh86 commented Aug 8, 2023

Did you check docs and existing issues?

  • I have read all the lazy.nvim docs
  • I have searched the existing issues of lazy.nvim
  • I have searched the existing issues of plugins related to this issue

Neovim version (nvim -v)

v0.10.0-dev 4a06de4

Operating system/version

Linux 5.15.90.1-microsoft-standard-WSL2

Describe the bug

To modify one plugin, I place the plugin under config.dev and set dev=true.
If there are other plugins that depend on that plugin, dev=true will be ignored and the plugin will be installed remotely under the control of lazy.nvim.

Steps To Reproduce

  1. Set dev = true for X plugin
  2. Set dependencies = { "X" } for Y plugin

Expected Behavior

X loaded from config.dev config.dev.path directory

Repro

-- DO NOT change the paths and don't remove the colorscheme
local root = vim.fn.fnamemodify("./.repro", ":p")

-- set stdpaths to use .repro
for _, name in ipairs({ "config", "data", "state", "cache" }) do
  vim.env[("XDG_%s_HOME"):format(name:upper())] = root .. "/" .. name
end

-- bootstrap lazy
local lazypath = root .. "/plugins/lazy.nvim"
if not vim.loop.fs_stat(lazypath) then
  vim.fn.system({ "git", "clone", "--filter=blob:none", "https://github.com/folke/lazy.nvim.git", lazypath, })
end
vim.opt.runtimepath:prepend(lazypath)

-- place local devs
local momijipath = root .. "/locals/momiji"
if not vim.loop.fs_stat(momijipath) then
  vim.fn.system({ "git", "clone",  "https://github.com/kyoh86/momiji.git", momijipath })
end

-- install plugins
local plugins = {{
  "kyoh86/momiji",
  dev = true,
  -- add any other plugins here
}, {
  "rebelot/heirline.nvim",
  dependencies = { "kyoh86/momiji" },

}}
require("lazy").setup(plugins, {
  root = root .. "/plugins",
  dev = { path = root .. "/locals" },
})
@kyoh86 kyoh86 added the bug Something isn't working label Aug 8, 2023
@abeldekat
Copy link
Contributor

abeldekat commented Aug 8, 2023

Hi @kyoh86

I can load your example with the following changes:

  1. Change name in dependencies:
local plugins = {
  { "kyo86/momiji", dev = true },
  {
    "rebelot/heirline.nvim",
    -- dependencies = { "kyo86/momiji" },
    dependencies = { "momiji" },
  },
}
  1. Change dev option in require:
require("lazy").setup(plugins, {
  root = root .. "/plugins",
  dev = { path = root .. "/locals/" },
})

@kyoh86
Copy link
Contributor Author

kyoh86 commented Aug 9, 2023

@abeldekat Thank you, the second one was my mistake.

@kyoh86
Copy link
Contributor Author

kyoh86 commented Aug 9, 2023

Could you redefine dependencies as just a place to present dependencies?
It is a simple model definition that all specs can have settings, wherever they are written, but if settings can be written in multiple layers, as in this case, we will be confused to read and understand the settings and change them later.
I think it would be simpler to specify only the name (like momiji) or full name (like kyoh86/momiji) in the dependencies, and the settings must be written in another place.
I don't know how much this will affect users, but it is a proposal that involves a disruptive change.
I respect the author's will, but I would appreciate your consideration.

@abeldekat
Copy link
Contributor

Hi @kyoh86,
You're welcome.

Regarding your question: As a user of lazy.nvim, I can only give you an opinion.
That approach would be very disruptive, limiting some important features. For example, lazy.nvim is able to detect unused plugins in the dependencies of a disabled parent plugin. Those plugins are also candidates for the clean command if they are not specified elsewhere. Using your approach, I think the user would have to manually mark all those plugins as disabled in their new corresponding top level specs.

I am thinking about the first change:

local plugins = {
  { "kyo86/momiji", dev = true },
  {
    "rebelot/heirline.nvim",
    -- dependencies = { "kyo86/momiji" },
    dependencies = { "momiji" },
  },
}

I contributed a little to the code that processes the spec, and for now I do not fully understand why using the full name in the dependencies breaks the dev setup.

I think it's a bug, further investigation is necessary.

@kyoh86
Copy link
Contributor Author

kyoh86 commented Aug 9, 2023

@abeldekat Thank you for your kind reply.
You are right, it would be difficult to mark plugins that are not dependencies if implemented as I suggested easily.
I could not think of a way to solve that problem.

And I also looked into the process, but could not find the root cause of this issue.
I'll wait for @folke to give us some input.

@abeldekat
Copy link
Contributor

abeldekat commented Aug 14, 2023

Hi @kyoh86,

I can narrow down your use case. The problem is not related to dependencies.

{
  { "kyoh86/momiji", dev = true }, -- parent spec, dir is "dev dir"
  { "kyoh86/momiji"}, -- new spec, dir is "plugin dir"
}

The last snippet in this example overrides the dir implicitly set in the first snippet.
That overriding mechanism is implicit, making it harder to understand. However, from the docs:

opts, dependencies, cmd, event, ft and keys are always merged with the parent spec. Any other property will override the property from the parent spec.

In your example, the dependencies property contains a spec, thus the same mechanism applies:

dependencies = { "kyoh86/momiji" },

If the dependencies property contains only a name, lazy.nvim has no need to create another spec. As a consequence, the dir of the dev setting is not overridden:

dependencies = { "momiji" },

The solution:

{
  {
    "rebelot/heirline.nvim",
    dependencies = { "kyoh86/momiji" },
  },
  { "kyoh86/momiji", dev = true },
}

EDIT
Or, maintaining the order in your example:

local plugins = {
  { "kyo86/momiji", dev = true },
  {
    "rebelot/heirline.nvim",
    -- dependencies = { "kyo86/momiji" },
    dependencies = { "momiji" },
  },
}

@max397574
Copy link
Contributor

imo this isn't a bug at all
makes completely sense that this option is overwriten from the dependencies section
you want to be able to completely express which dependency should be used (also e.g. branches) so it should be possible to write specs there and not forcing the user to write them elsewhere as suggested. I think in your case you should just use the plugin name and everything should work as expected.

@abeldekat
Copy link
Contributor

abeldekat commented Aug 14, 2023

Hi @max397574,

... so it should be possible to write specs there ...

Of course. I did not say anything in that regard.

..and not forcing the user to write them elsewhere as suggested...

That was not my intention at all though. I should have mentioned my first comment

In that comment I already gave the solution you mention:

local plugins = {
  { "kyo86/momiji", dev = true },
  {
    "rebelot/heirline.nvim",
    -- dependencies = { "kyo86/momiji" },
    dependencies = { "momiji" },
  },
}

I will add the above to the comment you are referring to.

... imo this isn't a bug at all...

I agree and I hope that the arguments in my comment illustrate this.

@kyoh86
Copy link
Contributor Author

kyoh86 commented Aug 14, 2023

@max397574 @abeldekat

Thank you all for your kind responses.
As you say, this does not seem to be a bug.
I understand the solution as well.

I use import= to separate the settings of the 100 or so plugins I have installed.
Therefore, dependencies are separated in various places, and the order of specs cannot be expressed clearly.
In my case, perhaps a style of specifying only names in dependencies is appropriate.

As a result, I do not get the advantage of being able to describe specs in dependencies.
It is also impossible to detect plug-ins that are no longer needed because they are no longer dependent.
But sadly, I can't think of simple structure that could benefit from this, so I think it's a fair result.

It seems that the spec in dependencies can only be beneficial in a limited cases.

  • Dependency settings are only written in one place.
  • Or all of the dependency setting is only the installation source (like "owner/repo"), with no other settings added.
  • Or the order of the descriptions is completely controllable.

@abeldekat
Copy link
Contributor

@kyoh86,

I think you can use the full potential of the dependencies field.
You can specify dev settings on require("lazy").setup(), using patterns:

Fragment of config:

dev = {
    -- directory where you store your local plugin projects
    path = "~/projects",
    ---@type string[] plugins that match these patterns will use your local versions instead of being fetched from GitHub
    patterns = {}, -- For example {"folke"}
    fallback = false, -- Fallback to git when local plugin doesn't exist
  },

Now, the order is irrelevant. Lazy.nvim automatically adds the dev marker on all snippets when one of the patterns occur in the url. See here
This saves you from adding the dev marker all over your config.

@kyoh86
Copy link
Contributor Author

kyoh86 commented Aug 14, 2023

@abeldekat
Wow! Thanks for the great solution!
I should have read the documentation carefully.
I will try to come up with a plugin development flow that uses this feature.

@abeldekat
Copy link
Contributor

@kyoh86,

You're welcome. Glad I could help.

@mehalter
Copy link

@abeldekat this is a very well put together comment: #982 (comment)

Although I do think this should be considered a bug or at least an unintentional implementation detail of the override mechanism. Basically from the documentation that you quoted:

opts, dependencies, cmd, event, ft and keys are always merged with the parent spec. Any other property will override the property from the parent spec.

I would completely expect this to be only explicitly set fields not that when a new spec is put in it is fully calculated out in place. I would expect that it would do all of the merging throughout the configuration and then at the end fill in the missing pieces. I totally understand the behavior that you are describing, but imagine the case where I specify dir explicitly and then later want to reference that plugin, it would appear that it would automatically calculate dir for that new "spec" rather than using my explicit definition that was given prior. It seems pretty unfortunate in this case how the things are put together.

Maybe this is just considered outside of the scope of lazy.nvim if it would take such a large rewrite of the methodology to change which I think is totally fine. I do think if that's the case, then the dev and dir field should probably just flat out be taken away since the arbitrary nesting of specs makes it basically impossible to effectively use them in an intelligible way. And then uses can just rely on using the dev option in the setup() call to do it and just not have the ability to set the specific directory for plugins.

@abeldekat
Copy link
Contributor

@mehalter,thanks! Would you agree to continue the conversation in your issue? At the moment, I am in doubt where to respond. Your thoughts resemble mine quite well I guess, see my comment in your issue 985

@mehalter
Copy link

I think now that the root cause is found to be the same, I think we should consolidate these as duplicates because I don't think they are different bugs now! Let me know what you think @abeldekat and I can just close mine out and drop a link to this thread to consolidate the conversation

@abeldekat
Copy link
Contributor

We have narrowed down the use cases in both these issues, thus I think we can save Folke some time.
I would suggest a new issue, an improvement/feature perhaps, in which we mention these two issues. That issue could contain the gist of our thoughts...

@abeldekat
Copy link
Contributor

Hi @kyoh86,

I created a new issue and would appreciate your feedback.
If you agree, this issue can be closed.

@max397574, if you have the time, please have a look at the new issue.

@kyoh86
Copy link
Contributor Author

kyoh86 commented Aug 15, 2023

@abeldekat Thank you for your kind guidance.
I am glad to see that an excellent discussion has begun. I will close this Issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants