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

Update core/init.lua, Putting .../mason/bin/ at the beginning of vim.env.PATH #2031

Merged
merged 1 commit into from
Jun 10, 2023

Conversation

TirtharajPramanik
Copy link
Contributor

Putting .../mason/bin/ in the beginning of vim.env.PATH

There is an issue with rust_analyzer, a mason plugin, not loading properly on macos ventura, neovim setup with NvChad default configs.
.../mason/bin/ is added at end of vim.env.PATH, causing a broken rust-analyzer from ~/.cargo/bin being loaded first.

I propose this change to search mason/bin first for plugins

Putting `.../mason/bin/` in the beginning of `vim.env.PATH`
@adamrp
Copy link

adamrp commented Jun 23, 2023

@siduck @TirtharajPramanik how do I prevent this from happening? The best I can do is get it to both prepend and append the path, but I cannot for the life of me figure out how to keep it from prepending it. The issue is that when I'm working with a virtual environment, I want the binaries from that virtual environment to take precedence, but it seems like this ensures that mason trumps them every time.

@siduck
Copy link
Member

siduck commented Jun 23, 2023

@adamrp just delete the ones downloaded from mason? 🤔

@TirtharajPramanik
Copy link
Contributor Author

TirtharajPramanik commented Jun 24, 2023

@adamrp You could write a script that moves specified plugins from mason/bin to a backup location and brings them back
or
You could position mason/bin in a suitable position in path for your workflow

Delete these lines in local config

-- ~/.config/nvim/lua/core/init.lua
59| -- add binaries installed by mason.nvim to path
60| local is_windows = vim.loop.os_uname().sysname == "Windows_NT"
61| vim.env.PATH = vim.fn.stdpath "data" .. "/mason/bin" .. (is_windows and ";" or ":") .. vim.env.PATH

Add mason/bin at the start of path in shell config file

# ~/.bashrc
export PATH="$HOME/.local/share/nvim/mason/bin/:$PATH"

I have tested this
inside a python3 virtualenv I have installed pyright
when activated virtualenv puts its bin/ at the start of path for current session,

before virtualenv
Screenshot 2023-06-25 at 04 50 01
after virtualenv
Screenshot 2023-06-25 at 04 49 08

@adamrp
Copy link

adamrp commented Jun 25, 2023

Please forgive me if my question is naive (I've used vim for a long time, but am new to neovim and nvchad), but why is this line in core/init.lua in the first place?

Having this line seems to break the "normal" flow where settings (including settings for plugins) sort of "cascade":

  • core/init.lua loads the default config
    • core/default_config.lua has this line which initializes an empty map for plugin configs
  • it then loads the custom/chadrc.lua (here)
    • this custom/chadrc.lua file can (IIUC should?) modify the default config -- including plugin configs -- as desired by the individual user (e.g., you and I have different customization needs for the mason plugin, and this would seem to me to be the "correct" place to make our respective modifications).

In this case, the mason plugin has an option in its config to control whether the mason/bin path is prepended or appended to the system PATH (or skipped). But modifying the PATH in core/init.lua masks the PATH setting in the mason config that I set in my custom/plugins.lua.

Again, I'm kinda new 😅 Intuitively, I would think that mason shouldn't receive "special treatment" in core/init.lua, and we should be able to modify the plugin's behavior in the "normal' way.

So my suggestion for NvChad/main branch would be:

  • Remove this line from core/init.lua entirely

For your use case @TirtharajPramanik, you could use a custom/plugins.lua file something like this:

local plugins = {
  {
    "williamboman/mason.nvim",
    opts = {
      PATH="prepend",  -- In my use case, I would use "append" here
      ensure_installed = {
        "rust-analyzer",
      },
    },
  },
}
return plugins

and combine it with a custom/chadrc.lua like:

---@type ChadrcConfig 
 local M = {}
 M.plugins = "custom.plugins"
 return M

@TirtharajPramanik
Copy link
Contributor Author

TirtharajPramanik commented Jun 25, 2023

@adamrp Thanks for this great analysis.
But I have issues replicating it. Does it work on your machine?

My configs

-- ~/.config/nvim/lua/core/init.lua
61| vim.env.PATH = vim.env.PATH .. (is_windows and ";" or ":") .. vim.fn.stdpath "data" .. "/mason/bin"
-- ~/.config/nvim/lua/custom/plugins.lua
local plugins = {
  {
    "williamboman/mason.nvim",
    opts = { PATH = "prepend" }
  }
}
return plugins

Resulting Package locations

Screenshot 2023-06-26 at 02 43 29 Screenshot 2023-06-26 at 02 43 36

Deleting this line

61| vim.env.PATH = vim.env.PATH .. (is_windows and ";" or ":") .. vim.fn.stdpath "data" .. "/mason/bin"

results in
Screenshot 2023-06-26 at 02 50 51

@TirtharajPramanik
Copy link
Contributor Author

TirtharajPramanik commented Jun 26, 2023

This setup function in mason.nvim contains logic for conditional path positioning

-- mason.nvim/lua/mason/init.lua
function M.setup(config)
...
    if settings.current.PATH == "prepend" then
        vim.env.PATH = path.bin_prefix() .. platform.path_sep .. vim.env.PATH
    elseif settings.current.PATH == "append" then
        vim.env.PATH = vim.env.PATH .. platform.path_sep .. path.bin_prefix()
    end
end

This setup function from mason is called inside this NvChad config function. link.

-- NvChad/lua/plugins/init.lua
default_plugins = {
  {
    "williamboman/mason.nvim",
    config = function(_, opts)
...
      require("mason").setup(opts)
    end,
  },
}
...
require("lazy").setup(default_plugins, config.lazy_nvim)

According to lazy.nvim docs

config is executed when the plugin loads. 

thus, logic for conditional path positioning not set when nvim first opens.

  • With PATH="prepend" option set, executing

    : =require("mason").setup(require(require("core.utils").load_config().plugins)[2])
    -- OR
    : Mason

    from nvim lua interpreter puts mason/bin/ at the start of path.

  • This initfunction, which will be called at startup of nvim, calls mason.setup function containing the logic for conditional path positioning.

    {
        "williamboman/mason.nvim",
    ...
        init = function(self)
          local config = vim.tbl_deep_extend("force", self._.super.opts(), self.opts)
          require("mason").setup(config).
        end,
      },

    Having PATH="prepend" option set, this setting puts mason/bin at start of path.

@adamrp @siduck Is this behaviour desirable? Or should PATH=prepend|append|skip be a separate option within NvChad.

Edit

As this doc suggests

Lazy-loaded plugins are automatically loaded when their Lua modules are required

calling require("mason") inside init function is same as eagerly loading mason or setting lazy=false

The key was to load mason "eagerly" (i.e., lazy=false in its config). link.

@adamrp
Copy link

adamrp commented Jun 26, 2023

First of all, thank you @TirtharajPramanik for your attention, I appreciate your help! 🏅

I just got this working the way I want it to, so I'll share what I did here. FYI I also am using a fork of NvChad (relevant branch here), so I pushed the working version there (relevant commit here).

In Summary:

  • Removed this from lua/core/init.lua
-- add binaries installed by mason.nvim to path
local is_windows = vim.loop.os_uname().sysname == "Windows_NT"
vim.env.PATH = vim.fn.stdpath "data" .. "/mason/bin" .. (is_windows and ";" or ":") .. vim.env.PATH
  • modified mason's entry in lua/custom/plugins.lua to this:
{
    "williamboman/mason.nvim",
    lazy=false,
    opts = {
      PATH="append",
      ensure_installed = {
        "pyright",
        "lua-language-server",
        "mypy",
        "flake8",
        "cfn-lint",
        "isort",
        "black",
      },
    },
  },
  • The important thing here was to set lazy=false

With this setup, the virtual environment's binaries will be preferred, but the mason binaries are used as a fallback. E.g.:

When I'm editing a python file that's not part of a virtual environment:

# echo $PATH
< ... snip ... >:/home/adam/.local/share/nvim/mason/bin

# which mypy
/home/adam/.local/share/nvim/mason/bin/mypy

Versus when I am in a virtual environment:

# echo $PATH
/home/adam/.cache/pypoetry/virtualenvs/sdh-api-backend-GuTxofRg-py3.10/bin:/home/adam/.pyenv/versions/sdh-api-backend/bin:/home/adam/.pyenv/libexec:/home/adam/.pyenv/plugins/python-build/bin:/home/adam/.pyenv/plugins/pyenv-virtualenv/bin:/home/adam/.pyenv/plugins/pyenv-update/bin:/home/adam/.pyenv/plugins/pyenv-doctor/bin:< ... snip ... >:/home/adam/.local/share/nvim/mason/bin

# which mypy
/home/adam/.cache/pypoetry/virtualenvs/sdh-api-backend-GuTxofRg-py3.10/bin/mypy

The key was to load mason "eagerly" (i.e., lazy=false in its config).

@siduck
Copy link
Member

siduck commented Jun 27, 2023

@adamrp please dont remove anything from default config, your nvchad will stop updating now. Just add whatever you want in the /custom/init.lua

@adamrp
Copy link

adamrp commented Jun 27, 2023

@siduck can you clarify --

  • Do you mean that that having the mason/bin in $PATH by this point in core/init.lua is required (whether it's appended or prepended)?
    • Looking at the code, I can't see why it would be needed, but if it is needed, can you explain? It feels "wrong" to modify the user's PATH in this way, especially since it masks the user's specified PATH setting for mason's setup function.
  • Or do you mean that git merge would fail?
  • Or something else?

I'm trying to understand if it makes sense to get that change into main or whether I need to write custom/init.lua code to remove mason/bin from the PATH.

@siduck
Copy link
Member

siduck commented Jun 27, 2023

@adamrp nvchad updates wont work properly if you make changes outside custom dir. Does removing the mason stuff in custom init.lua work?

@adamrp
Copy link

adamrp commented Jun 27, 2023

@siduck I can test that out at some point today, but can you respond to if/why the specific PATH manipulation is needed in init/core.lua in the first place? I'm thinking now more on a philosophical level rather than a practical/personal level -- seems like users should be able to rely on the plugin's (i.e. mason's) settings (after all, the reason mason has the PATH opt is so that we don't have to manually manipulate the PATH)

@adamrp
Copy link

adamrp commented Jun 27, 2023

FWIW I do see #2162 which (I think) would also solve my problem, but doesn't satisfy my curiosity about why the PATH manipulation is a requirement 😆

@siduck
Copy link
Member

siduck commented Jun 27, 2023

FWIW I do see #2162 which (I think) would also solve my problem, but doesn't satisfy my curiosity about why the PATH manipulation is a requirement laughing

because almost 99% of nvchad users download lsps from mason so obviously its download folder should be on path. I want you to check if you can fix this issue i.e append at the end of the path with custom/init.lua itself so we dont need that PR

@TirtharajPramanik

@adamrp
Copy link

adamrp commented Jun 27, 2023

@siduck I will do that experiment, I'm just trying to understand if/why it doesn't work to change nvchad's default mason config here from "skip" to "prepend" -- and then users like me can override that setting in their custom/plugins.lua if desired. EDIT: which would obviate the need to do any "special treatment" (string manipulation on PATH) in core/init.lua

@TirtharajPramanik
Copy link
Contributor Author

TirtharajPramanik commented Jun 27, 2023

@adamrp I have explored this option here.
It seem mason has to be loaded for this option to take effect.

@adamrp
Copy link

adamrp commented Jun 27, 2023

@TirtharajPramanik very true -- is it acceptable to eager-load it? See my draft PR here #2163

@TirtharajPramanik
Copy link
Contributor Author

TirtharajPramanik commented Jun 27, 2023

@adamrp
which can mask or even override the user's specified config in custom/plugins.lua.

for you concern of having two config options for the same functionality, I have noticed when both options in custom/chadrc.lua and plugins/configs/mason.lua are set to something other than skip both of them are executed.

i.e

--- custom/chadrc.lua    | --- plugins/configs/mason.lua
M.mason_path = "prepend" | PATH = "append"
PATH = ...mason/bin : $PATH : ...mason/bin

When any of them are skip only the other option takes effect and plugins/configs/mason.lua option is set to skip by default.

@adamrp
Copy link

adamrp commented Jun 27, 2023

@TirtharajPramanik interesting -- I can't speak to the behavior when it's in chadrc.lua versus custom/plugins.lua, but when it's in custom/plugins.lua (as in that draft PR #2163) it is only in the PATH exactly once, and in the position that the user requests.

@TirtharajPramanik
Copy link
Contributor Author

TirtharajPramanik commented Jun 27, 2023

I want you to check if you can fix this issue i.e append at the end of the path with custom/init.lua itself

  • Is this what you are asking for?

    -- custom/init.lua
    -- add binaries installed by mason.nvim to path
    local is_windows = vim.loop.os_uname().sysname == "Windows_NT"
    local mason_bin = vim.fn.stdpath "data" .. "/mason/bin"
    -- by default mason/bin is appended to path
    -- to prepend mason/bin to path swap vim.env.PATH and mason_bin
    vim.env.PATH = vim.env.PATH .. (is_windows and ";" or ":") .. mason_bin

    "What if somebody did not setup custom directories?"
    My implementation here could be flawed because setting mason/bin is essential for almost everybody's workflow
    and custom/ files are not required to be installed.

  • For autocommands I could not find a suitable event to hook up to.

  • Eagerly loading mason for its implementation of path handling is also a reasonable option.

  • And is having this option in custom/chadrc.lua viable?

    -- custom/chadrc.lua
    ---@type '"prepend"' | '"append"' | '"skip"'
    M.mason_path = "prepend"

    Another option could be utilizing the setting in plugins/configs/mason.lua:

    -- custom/configs/overrides.lua
    M.mason = {
      PATH = "prepend"
      ensure_installed = {

    Setting this to something other than skip may result in mason/bin put in path twice as mason also uses this setting for its path handling.

@adamrp
Copy link

adamrp commented Jun 27, 2023

I think taking the PATH manipulation out of core/init.lua -- and instead relying on mason's built-in implementation of path handling when its setup method is called -- is the "most minimally invasive" strategy from an NvChad user's perspective. I also think the logic is easier to follow because it's consistent with how other NvChad plugins are configured. The main tradeoff that I can identify is that mason has to be loaded lazy=false to make sure the PATH is set up correctly by the time a user starts editing a file, but IMHO that's a pretty low cost.

FWIW I think one issue with doing this sort of thing in chadrc is that it is (by default in core/init.lua) reloaded every time a buffer is written, so doing path manipulations there sounds tricky at best, and I think it makes more sense to do this path manipulation exactly once (when mason's setup function is called).

Is there something fundamentally wrong with what's in the draft PR #2163? Maybe eager-loading mason is a hard "no"?

@siduck
Copy link
Member

siduck commented Jun 28, 2023

image

this is how mason handles the path, not much different from ours

@adamrp
Copy link

adamrp commented Jun 28, 2023

@siduck it's not that different except that we (if I may include myself) are doing a string manipulation on PATH in core/init.lua outside of any interaction with the plugin's settings (which has the effect of potentially nullifying the user's explicitly expressed desire for that setting)

@siduck
Copy link
Member

siduck commented Jun 28, 2023

@siduck it's not that different except that we (if I may include myself) are doing a string manipulation on PATH in core/init.lua outside of any interaction with the plugin's settings (which has the effect of potentially nullifying the user's explicitly expressed desire for that setting)

just edit the path in custom/init.lua then

adamrp added a commit to adamrp/NvChad that referenced this pull request Jun 28, 2023
@adamrp
Copy link

adamrp commented Jun 28, 2023

Although I'm perplexed by the decision to deliberately ignore the user's plugin settings, I got the behavior I wanted by doing the following:

-- custom/plugins.lua
local plugins = {
  {
    "williamboman/mason.nvim",
    lazy=false,
    opts = {
      PATH="append",
      ensure_installed = {
        "pyright",
        "lua-language-server",
        "mypy",
        "flake8",
        "cfn-lint",
        "isort",
        "black",
      },
    },
  },
}
-- custom/chadrc.lua
 local M = {}
 M.plugins = "custom.plugins"
 return M
-- custom/init.lua
-- remove all occurrences of mason/bin from the path (it will be loaded by the plugin's setup function)
vim.env.PATH = string.gsub(vim.env.PATH, ".-/mason/bin:?", "")

Thank you both sincerely for your time and attention @siduck @TirtharajPramanik

@siduck
Copy link
Member

siduck commented Jun 28, 2023

@TirtharajPramanik i think the above solution should work and we dont need a separate option for this.

@adamrp i havent read the whole convos here, but why are you trying to remove mason paths from your env? 🤔

@TirtharajPramanik
Copy link
Contributor Author

TirtharajPramanik commented Jun 29, 2023

i think the above solution should work and we dont need a separate option for this.

Thank you for considering the feedback. To ensure a thorough evaluation before proceeding, I will keep this pull request open for one more day. During this time, I will conduct additional research to check for any existing occurrences of similar problems or alternative solutions online.

@adamrp
Copy link

adamrp commented Jun 29, 2023

@adamrp i havent read the whole convos here, but why are you trying to remove mason paths from your env?

@siduck I use null-ls together with mypy to do static type checking on my python projects (for my job, so they're big shared repositories).

  • Each python project has its own virtual environment
  • Each of the virtual environments has its own bespoke set of dependencies, including dev tools like mypy. It also has isort, flake8, black, etc. (other python style/lint/format tools), but mypy is the important one here because...
  • For most of those tools, it probably doesn't matter too much which executable is used
  • But for mypy, it's important that the executable from the virtual environment is used -- this is important because it "knows about" the virtual environment's dependencies whereas the executable installed by mason (since it's not part of the virtual env) does not
  • When the virtual environment is activated, it sets up PATH so that its binaries are preferred
  • But then, when I launch nvim + nvchad, core/init.lua puts the mason path at the start of the PATH, trumping the virtual environment's settings, and using the "wrong" instance of mypy
  • You might ask "why install mypy via mason in the first place, then?"
    • Well, I'm not always in a virtual environment. If I'm just editing e.g. /tmp/fooling_around.py, I do want it to fall back to mason's installation of mypy, which is why having it at the end of the PATH is what makes sense for me

So on a practical level, that's why I needed to make the change.

On a philosophical level, a few points:

  • I genuinely feel it's not good to modify users' PATH every time they launch their editor -- unless they ask for it. The user must be trusted to set up their own PATH for their individual needs. Modifying a user's PATH without their solicitation and without their knowledge feels like an attack 😆
  • Your (NvChad's) users are very very likely to get confused like I did when they specifically tell mason (via its normal plugin config setting for PATH) to PATH="skip" or PATH="append", only to find that it has apparently disobeyed them.
    • if they say "skip", it will be prepended to their path whether they like it or not
    • if they say "append", it will be both prepended and appended to their PATH -- baffling
  • AFAICT, NvChad does not treat any other plugin with this sort of "special handling," so IMHO it goes against (what I perceive to be) NvChad's philosophy of providing a standard way to manage plugins and their respective configurations -- we're used to there being a default config that can be overridden/merged with a custom config before it is loaded and its setup is called
    • The special handling for mason undermines this standard practice in a way that's surprising and unwelcome

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants