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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jan 25, 2025
Merged

fix: elixir installation failed #4144

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4a058cf
fix: elixir installation failed
roele Jan 23, 2025
352da8e
[autofix.ci] apply automated fixes
autofix-ci[bot] Jan 23, 2025
baf7df5
Merge branch 'main' into issues/4114
roele Jan 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/cli/install.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use crate::config::Config;
use crate::hooks::Hooks;
use crate::toolset::{InstallOptions, ResolveOptions, ToolRequest, ToolSource, Toolset};
use crate::ui::multi_progress_report::MultiProgressReport;
use crate::{config, hooks};
use crate::{config, env, hooks};
use eyre::Result;
use itertools::Itertools;

Expand Down Expand Up @@ -49,7 +49,10 @@ impl Install {
pub fn run(self) -> Result<()> {
let config = Config::try_get()?;
match &self.tool {
Some(runtime) => self.install_runtimes(&config, runtime)?,
Some(runtime) => {
env::TOOL_ARGS.write().unwrap().clone_from(runtime);
self.install_runtimes(&config, runtime)?
}
None => self.install_missing_runtimes(&config)?,
};
Ok(())
Expand Down
1 change: 1 addition & 0 deletions src/cli/use.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ impl Use {
if self.tool.is_empty() && self.remove.is_empty() {
self.tool = vec![self.tool_selector()?];
}
env::TOOL_ARGS.write().unwrap().clone_from(&self.tool);
let config = Config::try_get()?;
let mut ts = ToolsetBuilder::new()
.with_global_only(self.global)
Expand Down
3 changes: 2 additions & 1 deletion src/env.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::cli::args::{ENV_ARG, PROFILE_ARG};
use crate::cli::args::{ToolArg, ENV_ARG, PROFILE_ARG};
use crate::env_diff::{EnvDiff, EnvDiffOperation, EnvDiffPatches, EnvMap};
use crate::file::replace_path;
use crate::shell::ShellType;
Expand All @@ -14,6 +14,7 @@ use std::sync::RwLock;
use std::{path, process};

pub static ARGS: RwLock<Vec<String>> = RwLock::new(vec![]);
pub static TOOL_ARGS: RwLock<Vec<ToolArg>> = RwLock::new(vec![]);
#[cfg(unix)]
pub static SHELL: Lazy<String> = Lazy::new(|| var("SHELL").unwrap_or_else(|_| "sh".into()));
#[cfg(windows)]
Expand Down
14 changes: 14 additions & 0 deletions src/toolset/tool_request_set.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,20 @@ impl ToolRequestSetBuilder {
}
merge(trs, arg_ts);
}

let tool_args = env::TOOL_ARGS.read().unwrap();
let mut arg_trs = ToolRequestSet::new();
for arg in tool_args.iter() {
if let Some(tvr) = &arg.tvr {
arg_trs.add_version(tvr.clone(), &ToolSource::Argument);
} else if !trs.tools.contains_key(&arg.ba) {
// no active version, so use "latest"
let tr = ToolRequest::new(arg.ba.clone(), "latest", ToolSource::Argument)?;
arg_trs.add_version(tr, &ToolSource::Argument);
}
}
merge(trs, arg_trs);
Copy link
Owner

Choose a reason for hiding this comment

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

I worry slightly this may have unintended consequences if we later create a config that isn't supposed to be "complete" but just a subset, but I don't know of any obvious places to check. I think it's good that we have this limited to install/use which should limit the blast radius.

Not a big enough issue that I think we shouldn't merge this but something to keep in mind if we see issues later.

Copy link
Owner

Choose a reason for hiding this comment

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

the fact we don't have equivalent logic in the ToolsetBuilder might also be an issue, something to look out for later

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, since the issue is limited to install/use at this point i think its better to only have this in ToolRequestSet for now. ToolsetBuilder is used much more broadly and the blast radius would be huge.


Ok(())
}
}
Expand Down
Loading