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

Manage the installation folder path through env variable. #429

Closed
wants to merge 6 commits into from
Closed

Manage the installation folder path through env variable. #429

wants to merge 6 commits into from

Conversation

ghyatzo
Copy link
Contributor

@ghyatzo ghyatzo commented Oct 31, 2022

This started from #406.
While the install location of the various julia versions can be customised through the JULIA_DEPOT_PATH env var, the self installation folder was not, and instead the program relied on the current position of the running executable (or symlink to it).
This PR introduces the env var JULIAUP_SELF_HOME which controls the paths to the juliaup binaries and selfconfiguration files.

To avoid too much book keeping to manage an eventual custom location, I disabled the logic for letting the user type the location itself, and instead only rely on setting up the right env variable, similarly to how rustup does.

@davidanthoff
Copy link
Collaborator

I left some comments at the end of #406, which maybe are easier and more robust ways to solve this.

I think we should generally try to not introduce another env variable, at the end of the day that is another state that is stored somewhere that can get out of sync with the rest of the state and then we have to handle all the potential errors that can come about from that.

@ghyatzo
Copy link
Contributor Author

ghyatzo commented Nov 22, 2022

Regarding the book keeping part I agree, that is why I also proposed to modify the installation script to only be controlled by the env variable (similar to how rustup handles things). That way you don't have to keep track if the user selects a different folder and juggle a possible dual state, where your env variable points somewhere but the internal state chosen by the user points somewhere else. You basically keep part of the state (the self referential one) outside the program.

If a user has the need to specify a custom installation folder she can do so only through the env var, and It is mentioned during the installation script where you are presented the current installation configuration.

@davidanthoff
Copy link
Collaborator

I think the original problem is solved by #454, and beyond that I don't really see a benefit of introducing another state here via the env variable, so I'm tempted to close this, unless @ghyatzo thinks there are other benefits of this PR.

@ghyatzo
Copy link
Contributor Author

ghyatzo commented Nov 23, 2022

Late reply, differente time zone here.

The benefits of not relying on the current_exe() reference is that it enables running the juliaup program from anywhere within the filesystem (ie symlinking). Otherwise you are implicitly assuming that whichever folder the program is running from is the self home folder which can be wrong. #406 as an example.

The implemented changes don't really solve #406, since the real issue is the reference to paths.juliaupselfhome which calls let my_own_path = std::env::current_exe() which reference the wrong folder if running juliaup from a symlink (see #406 (comment)), In which case the program would try to delete non existant folders.

EDIT: May I also point out that the extra env variable, is completely optional in this PR. If you don't want to set it up you can, and the program goes on with the default settings. So It is in practice a completely transparent PR.

@davidanthoff
Copy link
Collaborator

Ah, I had assumed that current_exe() would always return the path of the binary, even if the binary was launched via a symlink. Is that not the case?

@ghyatzo
Copy link
Contributor Author

ghyatzo commented Nov 23, 2022

I too had assumed that it would not follow symlinks, but apparently, it is platform dependant also here. I am on Mac so in my case it doesn't follow the symlinks. Odd that in the documentation they don't specify what platform does what though.

@davidanthoff
Copy link
Collaborator

So maybe the right strategy would be to a) check whether the path returned by current_exe is a symbolic link, b) if it is, dereference it to find the target of the link, and then c) delete things relative to that path?

Or maybe there is just something similar to current_exe that always returns the actual path of the binary, not the path of the symbolic link?

@ghyatzo
Copy link
Contributor Author

ghyatzo commented Nov 24, 2022

I found the following example to be working:
test.rs

fn main() {
    let mypath = std::env::current_exe();
    let _actual_path = match std::fs::read_link(mypath.as_ref().unwrap()) {

        Ok(link) => {
            println!("path is a link pointing to {:?}", link)
        }
        Err(_) => {
            println!("path points to real exec {:?}", mypath)
        }
    };
}

and with

> rustc test.rs
> ln test hard_link
> ln -s test soft_link
> ./test 
path points to real exec Ok(".../test")
> ./hard_link
path points to real exec Ok(".../hard_link")
> ./soft_link
path is a link pointing to "test"

The only annoying thing is that read_link does not necessarily return an absolute path, but most likely a relative one.
The relative path is relative to the symlink, so we can use current_exe() to obtain the path of the symlink and join it with the relative path from the symlink contents to reconstruct the real path.

@ghyatzo
Copy link
Contributor Author

ghyatzo commented Nov 24, 2022

Even better, I found this which resolves symlinks.

so we can just call the current_exe

let mypath = std::fs::canonicalize(std::env::current_exe().expect("Can't find current exe"));

and with this calling the symlinks prints the right executable. (At least on MacOs, don't have access to machines with other OSes to test)

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.

2 participants