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

Centralize where paths are discovered #915

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

Conversation

davidanthoff
Copy link
Collaborator

Alterantive to #886.

@ghyatzo I started to look into it, and then got carried away ;) I think this should be correct, but I'm also a bit nervous as any error in any of this would really be bad... If you could take a careful look if this all looks right to you that would be amazing.

The general idea now is that juliaupselfexecfolder is the folder where the juliaup binary is located and juliaupselfexec is the path to the juliaup binary itself. And all paths should be canonical.

There is one thing I don't fully understand, I'll leave a comment at that location.

.parent()
.ok_or_else(|| anyhow!("Could not determine parent."))?
.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.

@ghyatzo
Copy link
Contributor

ghyatzo commented Apr 30, 2024

Nice to have this alternative!
I took a look, it all seems good, and actually clears up a lot of the code, which is nice.
I didn't want to touch the various features flags, or signatures, but happy that you did it, it feels better to have a single storage.

first a minor comment: probably still having the logic to get the exec path in a separate function can be beneficial for maintainability.

On another note, I am really uneasy about the get_bin_dir() function and the JULIAUP_BIN_DIR environmental variable.
I commented about it exensively in my PR.
That function get_bin_dir really feels out of place, and the JULIAUP_BIN_DIR is only used there as far as I can tell.

I would also really know what you think about the proposal of just instead using JULIAUP_DEPOT_PATH as the real "authority" and everything else can follow from that. (check the comment in #886)!

@ghyatzo
Copy link
Contributor

ghyatzo commented Jun 16, 2024

Any news regarding this? or comments in #886?

Copy link
Contributor

@ghyatzo ghyatzo left a comment

Choose a reason for hiding this comment

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

I might have just now realised I was marked as reviewer on this PR :P
Added a comment reporting some doubts I had from my own PR about this topic.
But other than that I like this a lot.
Sorry for the long wait!

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

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.

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