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

Centralize where paths are discovered #915

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Centralize where paths are discovered #915

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 7 additions & 7 deletions src/bin/juliainstaller.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,17 +379,17 @@
}
}

let juliaupselfbin = install_choices.install_location.join("bin");
let juliaupselfexecfolder = install_choices.install_location.join("bin");

trace!("Set juliaupselfbin to `{:?}`", juliaupselfbin);
trace!("Set juliaupselfexecfolder to `{:?}`", juliaupselfexecfolder);

println!("Now installing Juliaup");

if paths.juliaupconfig.exists() {
std::fs::remove_file(&paths.juliaupconfig).unwrap();
}

std::fs::create_dir_all(&juliaupselfbin)
std::fs::create_dir_all(&juliaupselfexecfolder)
.with_context(|| "Failed to create install folder for Juliaup.")?;

let juliaup_target = get_juliaup_target();
Expand All @@ -411,7 +411,7 @@
)
})?;

download_extract_sans_parent(&new_juliaup_url.to_string(), &juliaupselfbin, 0)?;
download_extract_sans_parent(&new_juliaup_url.to_string(), &juliaupselfexecfolder, 0)?;

{
let new_selfconfig_data = JuliaupSelfConfig {
Expand Down Expand Up @@ -445,7 +445,7 @@
.sync_all()
.with_context(|| "Failed to write config data to disc.")?;

paths.juliaupselfbin = juliaupselfbin.clone();
paths.juliaupselfexecfolder = juliaupselfexecfolder.clone();
paths.juliaupselfconfig = self_config_path.clone();
}

Expand All @@ -466,15 +466,15 @@

run_command_default(&args.default_channel, &paths)
.with_context(|| "Failed to run `run_command_default`.")?;

Check warning on line 469 in src/bin/juliainstaller.rs

View workflow job for this annotation

GitHub Actions / Rustfmt Check 

Diff in /home/runner/work/juliaup/juliaup/src/bin/juliainstaller.rs
let symlink_path = juliaupselfbin.join("julia");
let symlink_path = juliaupselfexecfolder.join("julia");

std::os::unix::fs::symlink(juliaupselfbin.join("julialauncher"), &symlink_path).with_context(
std::os::unix::fs::symlink(juliaupselfexecfolder.join("julialauncher"), &symlink_path).with_context(
|| {
format!(
"failed to create symlink `{}`.",
symlink_path.to_string_lossy()
)

Check warning on line 477 in src/bin/juliainstaller.rs

View workflow job for this annotation

GitHub Actions / Rustfmt Check 

Diff in /home/runner/work/juliaup/juliaup/src/bin/juliainstaller.rs
},
)?;

Expand Down
39 changes: 9 additions & 30 deletions src/bin/julialauncher.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
use console::Term;
use itertools::Itertools;
use juliaup::config_file::{load_config_db, JuliaupConfig, JuliaupConfigChannel};
use juliaup::global_paths::get_paths;
use juliaup::global_paths::{get_paths, GlobalPaths};
use juliaup::jsonstructs_versionsdb::JuliaupVersionDB;
use juliaup::versions_file::load_versions_db;
#[cfg(not(windows))]
Expand All @@ -22,34 +22,19 @@
msg: String,
}

fn get_juliaup_path() -> Result<PathBuf> {
let my_own_path = std::env::current_exe()
.with_context(|| "std::env::current_exe() did not find its own path.")?
.canonicalize()
.with_context(|| "Failed to canonicalize the path to the Julia launcher.")?;

let juliaup_path = my_own_path
.parent()
.unwrap() // unwrap OK here because this can't happen
.join(format!("juliaup{}", std::env::consts::EXE_SUFFIX));

Ok(juliaup_path)
}

fn do_initial_setup(juliaupconfig_path: &Path) -> Result<()> {
fn do_initial_setup(juliaupconfig_path: &Path, path: &GlobalPaths) -> Result<()> {
if !juliaupconfig_path.exists() {
let juliaup_path = get_juliaup_path().with_context(|| "Failed to obtain juliaup path.")?;

std::process::Command::new(juliaup_path)
std::process::Command::new(&path.juliaupselfexec)
.arg("46029ef5-0b73-4a71-bff3-d0d05de42aac") // This is our internal command to do the initial setup
.status()
.with_context(|| "Failed to start juliaup for the initial setup.")?;
}
Ok(())
}

Check warning on line 34 in src/bin/julialauncher.rs

View workflow job for this annotation

GitHub Actions / Rustfmt Check 

Diff in /home/runner/work/juliaup/juliaup/src/bin/julialauncher.rs
fn run_versiondb_update(
config_file: &juliaup::config_file::JuliaupReadonlyConfigFile,
paths: &GlobalPaths
) -> Result<()> {
use chrono::Utc;
use std::process::Stdio;
Expand All @@ -67,10 +52,7 @@
};

if should_run {
let juliaup_path =
get_juliaup_path().with_context(|| "Failed to obtain juliaup path.")?;

std::process::Command::new(juliaup_path)
std::process::Command::new(&paths.juliaupselfexec)
.args(["0cf1528f-0b15-46b1-9ac9-e5bf5ccccbcf"])
.stdout(Stdio::null())
.stderr(Stdio::null())
Expand All @@ -81,10 +63,10 @@
}

Ok(())
}

Check warning on line 66 in src/bin/julialauncher.rs

View workflow job for this annotation

GitHub Actions / Rustfmt Check 

Diff in /home/runner/work/juliaup/juliaup/src/bin/julialauncher.rs

#[cfg(feature = "selfupdate")]
fn run_selfupdate(config_file: &juliaup::config_file::JuliaupReadonlyConfigFile) -> Result<()> {
fn run_selfupdate(config_file: &juliaup::config_file::JuliaupReadonlyConfigFile, paths: &GlobalPaths) -> Result<()> {
use chrono::Utc;
use std::process::Stdio;

Expand All @@ -102,10 +84,7 @@
};

if should_run {
let juliaup_path =
get_juliaup_path().with_context(|| "Failed to obtain juliaup path.")?;

std::process::Command::new(juliaup_path)
std::process::Command::new(paths.juliaupselfexec)
.args(["self", "update"])
.stdout(Stdio::null())
.stderr(Stdio::null())
Expand Down Expand Up @@ -289,7 +268,7 @@

let paths = get_paths().with_context(|| "Trying to load all global paths.")?;

do_initial_setup(&paths.juliaupconfig)
do_initial_setup(&paths.juliaupconfig, &paths)
.with_context(|| "The Julia launcher failed to run the initial setup steps.")?;

let config_file = load_config_db(&paths)
Expand Down Expand Up @@ -422,10 +401,10 @@

let mut child_process = std::process::Command::new(julia_path)
.args(&new_args)
.spawn()

Check warning on line 404 in src/bin/julialauncher.rs

View workflow job for this annotation

GitHub Actions / Rustfmt Check 

Diff in /home/runner/work/juliaup/juliaup/src/bin/julialauncher.rs
.with_context(|| "The Julia launcher failed to start Julia.")?; // TODO Maybe include the command we actually tried to start?

run_versiondb_update(&config_file).with_context(|| "Failed to run version db update")?;
run_versiondb_update(&config_file, &paths).with_context(|| "Failed to run version db update")?;

run_selfupdate(&config_file).with_context(|| "Failed to run selfupdate.")?;

Expand Down
2 changes: 1 addition & 1 deletion src/command_config_modifypath.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ pub fn run_command_config_modifypath(
}

if value {
add_binfolder_to_path_in_shell_scripts(&paths.juliaupselfbin)?;
add_binfolder_to_path_in_shell_scripts(&paths.juliaupselfexecfolder)?;
} else {
remove_binfolder_from_path_in_shell_scripts()?;
}
Expand Down
9 changes: 1 addition & 8 deletions src/command_selfupdate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
use crate::operations::{download_extract_sans_parent, download_juliaup_version};
use crate::utils::get_juliaserver_base_url;
use crate::{get_juliaup_target, get_own_version};
use anyhow::{anyhow, bail};

Check failure on line 11 in src/command_selfupdate.rs

View workflow job for this annotation

GitHub Actions / check-juliaup (x86_64-apple-darwin) 

unused import: `anyhow`

update_version_db(paths).with_context(|| "Failed to update versions db.")?;

Expand Down Expand Up @@ -71,19 +71,12 @@
)
})?;

let my_own_path = std::env::current_exe()
.with_context(|| "Could not determine the path of the running exe.")?;

let my_own_folder = my_own_path
.parent()
.ok_or_else(|| anyhow!("Could not determine parent."))?;

eprintln!(
"Found new version {} on channel {}.",
version, juliaup_channel
);

download_extract_sans_parent(&new_juliaup_url.to_string(), &my_own_folder, 0)?;
download_extract_sans_parent(&new_juliaup_url.to_string(), &path.juliaupselfexecfolder, 0)?;

Check failure on line 79 in src/command_selfupdate.rs

View workflow job for this annotation

GitHub Actions / check-juliaup (x86_64-apple-darwin) 

expected value, found built-in attribute `path`
eprintln!("Updated Juliaup to version {}.", version);
}

Expand Down
21 changes: 13 additions & 8 deletions src/global_paths.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::get_juliaup_target;
#[cfg(feature = "selfupdate")]
use anyhow::Context;

Check failure on line 3 in src/global_paths.rs

View workflow job for this annotation

GitHub Actions / check-juliaup (x86_64-apple-darwin) 

unused import: `anyhow::Context`
use anyhow::{anyhow, bail, Result};
use std::path::PathBuf;
pub struct GlobalPaths {
Expand All @@ -12,8 +12,8 @@
pub juliaupselfhome: PathBuf,
#[cfg(feature = "selfupdate")]
pub juliaupselfconfig: PathBuf,
#[cfg(feature = "selfupdate")]
pub juliaupselfbin: PathBuf,
pub juliaupselfexecfolder: PathBuf,
pub juliaupselfexec: PathBuf,
}

fn get_juliaup_home_path() -> Result<PathBuf> {
Expand Down Expand Up @@ -54,18 +54,23 @@
}

pub fn get_paths() -> Result<GlobalPaths> {
use anyhow::Context;

Check failure on line 57 in src/global_paths.rs

View workflow job for this annotation

GitHub Actions / check-juliaup (x86_64-apple-darwin) 

the item `Context` is imported redundantly

let juliauphome = get_juliaup_home_path()?;

#[cfg(feature = "selfupdate")]
let my_own_path = std::env::current_exe()
.with_context(|| "Could not determine the path of the running exe.")?;
.with_context(|| "std::env::current_exe() did not find its own path.")?
.canonicalize()
.with_context(|| "Failed to canonicalize the path to the Julia launcher.")?;

#[cfg(feature = "selfupdate")]
let juliaupselfbin = my_own_path
let juliaupselfexecfolder = my_own_path
.parent()
.ok_or_else(|| anyhow!("Could not determine parent."))?

Check warning on line 68 in src/global_paths.rs

View workflow job for this annotation

GitHub Actions / Rustfmt Check 

Diff in /home/runner/work/juliaup/juliaup/src/global_paths.rs
.to_path_buf();

let juliaupselfexec = juliaupselfexecfolder
.join(format!("juliaup{}", std::env::consts::EXE_SUFFIX));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So I don't really understand why we are doing this, i.e. why we start with the path to the binary, then take the parent, and then reconstruct the path to the binary here. BUT, that is how the code was before, and I'm nervous about changing it... And this might have been related to deal with some symbolic links stuff that I don't fully understand?

Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly I struggle to see the point of it as well. The only scenario I can think of for the reason of that, is that in windows current_exe doesn't return (maybe it didn't in the past?) the .exe suffix.
Dealing with symbolic links should be handled by .canonicalize() now anyway.


let juliaupconfig = juliauphome.join("juliaup.json");

let versiondb = juliauphome.join(format!("versiondb-{}.json", get_juliaup_target()));
Expand All @@ -92,7 +97,7 @@
juliaupselfhome,
#[cfg(feature = "selfupdate")]
juliaupselfconfig,
#[cfg(feature = "selfupdate")]
juliaupselfbin,
juliaupselfexecfolder,
juliaupselfexec,
})
}
21 changes: 7 additions & 14 deletions src/operations.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,13 +289,11 @@
return Ok(());
}

// TODO At some point we could put this behind a conditional compile, we know

Check warning on line 292 in src/operations.rs

View workflow job for this annotation

GitHub Actions / Rustfmt Check 

Diff in /home/runner/work/juliaup/juliaup/src/operations.rs
// that we don't ship a bundled version for some platforms.
let full_version_string_of_bundled_version = get_bundled_julia_version();
let my_own_path = std::env::current_exe()?;
let path_of_bundled_version = my_own_path
.parent()
.unwrap() // unwrap OK because we can't get a path that does not have a parent
let path_of_bundled_version = &paths
.juliaupselfexecfolder
.join("BundledJulia");

let child_target_foldername = format!("julia-{}", fullversion);
Expand Down Expand Up @@ -575,8 +573,8 @@
Ok(())
}

pub fn remove_symlink(symlink_name: &String) -> Result<()> {
let symlink_path = get_bin_dir()
pub fn remove_symlink(symlink_name: &String, paths: &GlobalPaths) -> Result<()> {
let symlink_path = get_bin_dir(paths)
Copy link
Contributor

@ghyatzo ghyatzo Aug 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

See comment (second half): #886 (comment)
In short: I don't get why the get_bin_dir uses an undocumented environment variable JULIAUP_BIN_DIR.
If we want to depend on current_exe() then that makes JULIAUP_BIN_DIR a potential source of headaches, since almost by definition, the path obtained by current_exe() is the actual JULIAUP_BIN_DIR (unless I am not seeing the use case for it)

If the JULIAUP_BIN_DIR was exclusively introduced (in #165 ) as a kitchen sink for various versions' symlinks then I guess it's not such a big deal. But I must admit I still find it confusing, especially since it is not documented and so it seems like a left over from a past approach?

Also, using the somewhat arbitrary~/.local/bin as a default might not be ideal, Ideally as default we should just use the juliaupselfexec also for symlinks no? and then if somebody wants them inside ~/.local/bin then he can use the JULIAUP_BIN_DIR instead.
But at that point I would also expect to have juliaup or default julia symlink inside JULIAUP_BIN_DIR, which fights with the authority of current_exe() we are using now.

.with_context(|| "Failed to retrieve binary directory while trying to remove a symlink.")?
.join(symlink_name);

Expand All @@ -597,7 +595,7 @@
symlink_name: &String,
paths: &GlobalPaths,
) -> Result<()> {
let symlink_folder = get_bin_dir()

Check failure on line 598 in src/operations.rs

View workflow job for this annotation

GitHub Actions / check-juliaup (x86_64-apple-darwin) 

this function takes 1 argument but 0 arguments were supplied

Check failure on line 598 in src/operations.rs

View workflow job for this annotation

GitHub Actions / test-juliaup (x86_64-unknown-linux-gnu) 

this function takes 1 argument but 0 arguments were supplied

Check failure on line 598 in src/operations.rs

View workflow job for this annotation

GitHub Actions / test-juliaup (x86_64-apple-darwin) 

this function takes 1 argument but 0 arguments were supplied
.with_context(|| "Failed to retrieve binary directory while trying to create a symlink.")?;

let symlink_path = symlink_folder.join(symlink_name);
Expand Down Expand Up @@ -707,15 +705,10 @@
}

#[cfg(feature = "selfupdate")]
pub fn install_background_selfupdate(interval: i64) -> Result<()> {
pub fn install_background_selfupdate(interval: i64, paths: &GlobalPaths) -> Result<()> {
use itertools::Itertools;
use std::process::Stdio;

let own_exe_path = std::env::current_exe()
.with_context(|| "Could not determine the path of the running exe.")?;

let my_own_path = own_exe_path.to_str().unwrap();

match std::env::var("WSL_DISTRO_NAME") {
// This is the WSL case, where we schedule a Windows task to do the update
Ok(val) => {
Expand All @@ -728,10 +721,10 @@
&interval.to_string(),
"/tn",
&format!("Juliaup self update for WSL {} distribution", val),
"/f",

Check warning on line 724 in src/operations.rs

View workflow job for this annotation

GitHub Actions / Rustfmt Check 

Diff in /home/runner/work/juliaup/juliaup/src/operations.rs
"/it",
"/tr",
&format!("wsl --distribution {} {} self update", val, my_own_path),
&format!("wsl --distribution {} {} self update", val, &paths.juliaupselfexec),

Check failure on line 727 in src/operations.rs

View workflow job for this annotation

GitHub Actions / check-juliaup (x86_64-apple-darwin) 

`PathBuf` doesn't implement `std::fmt::Display`
])
.output()
.with_context(|| "Failed to create new Windows task for juliaup.")?;
Expand All @@ -748,7 +741,7 @@
.chain([
&format!(
"*/{} * * * * {} 4c79c12db1d34bbbab1f6c6f838f423f",
interval, my_own_path
interval, &paths.juliaupselfexec

Check failure on line 744 in src/operations.rs

View workflow job for this annotation

GitHub Actions / check-juliaup (x86_64-apple-darwin) 

`PathBuf` doesn't implement `std::fmt::Display`
),
"",
])
Expand Down
12 changes: 5 additions & 7 deletions src/utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
use anyhow::{anyhow, bail, Context, Result};
use anyhow::{bail, Context, Result};
use semver::{BuildMetadata, Version};
use std::path::PathBuf;
use url::Url;

use crate::global_paths::GlobalPaths;

pub fn get_juliaserver_base_url() -> Result<Url> {
let base_url = if let Ok(val) = std::env::var("JULIAUP_SERVER") {
if val.ends_with('/') {
Expand Down Expand Up @@ -45,7 +47,7 @@ pub fn get_julianightlies_base_url() -> Result<Url> {
Ok(parsed_url)
}

pub fn get_bin_dir() -> Result<PathBuf> {
pub fn get_bin_dir(paths: &GlobalPaths) -> Result<PathBuf> {
let entry_sep = if std::env::consts::OS == "windows" {
';'
} else {
Expand All @@ -63,11 +65,7 @@ pub fn get_bin_dir() -> Result<PathBuf> {
path
}
Err(_) => {
let mut path = std::env::current_exe()
.with_context(|| "Could not determine the path of the running exe.")?
.parent()
.ok_or_else(|| anyhow!("Could not determine parent."))?
.to_path_buf();
let mut path = paths.juliaupselfexecfolder.clone();

if let Some(home_dir) = dirs::home_dir() {
if !path.starts_with(&home_dir) {
Expand Down
Loading