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

Jump to bottom

Inconsistent DENO_INSTALL_ROOT behaviour #26442

Open
mahtaran opened this issue Oct 21, 2024 · 2 comments · May be fixed by #27459
Open

Inconsistent DENO_INSTALL_ROOT behaviour #26442

mahtaran opened this issue Oct 21, 2024 · 2 comments · May be fixed by #27459
Labels
bug Something isn't working correctly good first issue Good for newcomers install

Comments

@mahtaran
Copy link

mahtaran commented Oct 21, 2024 •
edited
Loading

Version: Deno 2.0.0

As stated in the documentation, DENO_INSTALL_ROOT should default to $HOME/.deno/bin. However, in get_installer_root, it defaults to simply $HOME/.deno (

deno/cli/tools/installer.rs

Lines 117 to 137 in 473e306

fn get_installer_root() -> Result<PathBuf, io::Error> {
if let Ok(env_dir) = env::var("DENO_INSTALL_ROOT") {
if !env_dir.is_empty() {
return canonicalize_path_maybe_not_exists(&PathBuf::from(env_dir));
}
}
// Note: on Windows, the $HOME environment variable may be set by users or by
// third party software, but it is non-standard and should not be relied upon.
let home_env_var = if cfg!(windows) { "USERPROFILE" } else { "HOME" };
let mut home_path =
env::var_os(home_env_var)
.map(PathBuf::from)
.ok_or_else(|| {
io::Error::new(
io::ErrorKind::NotFound,
format!("${home_env_var} is not defined"),
)
})?;
home_path.push(".deno");
Ok(home_path)
}
), and the bin is only added later (

deno/cli/tools/installer.rs

Lines 212 to 218 in 473e306

let cwd = std::env::current_dir().context("Unable to get CWD")?;
let root = if let Some(root) = uninstall_flags.root {
canonicalize_path_maybe_not_exists(&cwd.join(root))?
} else {
get_installer_root()?
};
let installation_dir = root.join("bin");
,

deno/cli/tools/installer.rs

Lines 430 to 436 in 473e306

let cwd = std::env::current_dir().context("Unable to get CWD")?;
let root = if let Some(root) = &install_flags_global.root {
canonicalize_path_maybe_not_exists(&cwd.join(root))?
} else {
get_installer_root()?
};
let installation_dir = root.join("bin");
). This causes some weird behaviour when DENO_INSTALL_ROOT is set (for example, if it is set to $HOME/.deno/bin, packages will be installed to $HOME/.deno/bin/bin).
@lucacasonato lucacasonato added bug Something isn't working correctly good first issue Good for newcomers install labels Oct 21, 2024
@dovakin0007 dovakin0007 mentioned this issue Oct 21, 2024
Open
@BLANKatGITHUB
Copy link

BLANKatGITHUB commented Nov 9, 2024 •
edited
Loading

well I kind of understand the issue and want to tackle.
By seeing how detailed the issue is thanks to mahtaran, I think the main issue is the appending of .bin later instead of appending it directly inside

I think we should remove
let installation_dir = root.join("bin");

to

let installation_dir = root;

or replace root with installation_dir getting the output directly from if let .

@BLANKatGITHUB
Copy link

After this we can change get_installer_root function to append ".deno/bin" instead of ".deno"

@mahtaran mahtaran linked a pull request Dec 24, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working correctly good first issue Good for newcomers install
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

💥 Correct the behaviour of DENO_INSTALL_ROOT within (un)install commands mahtaran/deno
3 participants
@lucacasonato @mahtaran @BLANKatGITHUB