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

fix: elixir installation failed #4144

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: elixir installation failed #4144

wants to merge 2 commits into from

Conversation

roele
Copy link
Contributor

@roele roele commented Jan 17, 2025

Fixes #4114

The dependency_toolset function should not rely on dependencies in the Config to build the environment for the CmdLineRunner, especially when used with install.

Since this seems specific to Elixir so far and the method is used in other contexts, its probably best to keep the solution exclusive to elixir. If necessary we can move it up to the trait later. WDYT?

@jdx
Copy link
Owner

jdx commented Jan 17, 2025

doesn't this effectively make it so we'd always be using latest for dependencies and not the version the user configured?

perhaps I'm a bit tired but this sentence sounds like the exact opposite of how things should behave:

The dependency_toolset function should not rely on dependencies in the Config to build the environment for the CmdLineRunner, especially when used with install.

@roele
Copy link
Contributor Author

roele commented Jan 17, 2025

Yes that's ugly but get_dependencies only returns strings (eg "erlang") and with nothing in the config its the only option.
(might be wrong haven't looked at the InstallContext which might contain the version of the dependency)

Your're right, the current implementation seems correct but does not take into account that dependencies might not be in the config (unless i'm missing something). An install does not add them and in case of use, which also does not seem to work, it might be that the config does not yet reflect the latest state during installation of multiple tools.

Maybe we need something more sophisticated, that tries the config first and falls back to latest versions.

@jdx
Copy link
Owner

jdx commented Jan 17, 2025

Your're right, the current implementation seems correct but does not take into account that dependencies might not be in the config (unless i'm missing something). An install does not add them and in case of use, which also does not seem to work, it might be that the config does not yet reflect the latest state during installation of multiple tools.

it sounds like you're misunderstanding the correct behavior. It should be possible for a user to install erlang with homebrew and elixir with mise without also using mise to manage erlang.

@roele
Copy link
Contributor Author

roele commented Jan 17, 2025

Ah I see... that complicates things. If erlang is installed via brew it will already be on the PATH so this issue will not surface.

The culprit is the following which is passed to the verify function to check the version of elixir and should put erlang on the PATH.
https://github.com/jdx/mise/blob/1b5e7406ad969e8fc461d088beaf7886111bfbc3/src/backend/mod.rs#L601C1-L604C6

So i guess we need to check if the dependencies are part of the ToolSet (in InstallContext?) and add them, otherwise assume they're installed already (via brew or anyhow)?

I'll dig a bit more towards a better solution later today. Lazy me would be tempted to just remove the verify function as erlang also does not have one (which makes sense since version check is a pain) 😂

@jdx
Copy link
Owner

jdx commented Jan 17, 2025

yes, I think you're correct. The issue is that we always load the config from the config files and we're not including something that gets passed through command line arguments. Threading this through might be challenging.

@roele
Copy link
Contributor Author

roele commented Jan 21, 2025

Played around a bit more with this but it seems a bit tricky and ugly if you ask me.

The Config is immutably shared across threads, therefore installed tools are not reflected and use will also not have erlang exposed in the environment.

Passing down the tools from the arguments can be done relatively easily via InstallContext but requires additional methods dependency_env_with_ctx and dependency_toolset_with_ctx as the context is not always available and the methods are used in quite a few places already. This enables to resolve the tools that are defined as dependency and passed in as arguments.

One use-case that is not taken into account is when erlang is mananged via mise but not active. Installation of elixir will also fail in this case but i assume that is correct behaviour. It would also fail if erlang is not managed by mise and NOT on the PATH. Still it might be confusing for users.

@jdx
Copy link
Owner

jdx commented Jan 21, 2025

I've thought a few things that Config should not be immutable and that it instead should be based on the arguments sent in always but this is difficult since occasionally we have 1-off Configs that are completely different than the main one.

I agree with your assessment, I wonder if we should have 1 global Config that is based on the arguments that we could reuse here but only use it in certain situations, like a GLOBAL_CONFIG OnceLock or something.

That's definitely easier said than done though. Maybe as a stop-gap we could have a global mutex for tool arguments that we could populate when mise install starts which would then be used to optionally populate parts of the Config struct? Still not an easy solution but maybe something along those lines? I feel like that would make things slightly easier since you wouldn't need to drill down through the InstallContext and include this reference everywhere.

Or perhaps I'm wrong and the install context should be passed around to more places than we currently use it.

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.

Elixir installation failed
2 participants