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

canonicalize the current_exe result when self updating, so that it works with symlinking. #886

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ghyatzo
Copy link
Contributor

@ghyatzo ghyatzo commented Apr 5, 2024

Fixes #885,

It centralises the resolution of the self path into a single function. Consolidated all the locations which queried the self path to call that function for better maintainability and consistency.

this change could also help towards #844, giving better support to symlinks and therefore custom locations

Copy link
Collaborator

@davidanthoff davidanthoff left a comment

Choose a reason for hiding this comment

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

I like it in general, but maybe the right strategy here would be to just store the path to the bin and the parent folder in the GlobalPaths structure and then use that everywhere? That seems more consistent with the current design.

@ghyatzo
Copy link
Contributor Author

ghyatzo commented Apr 30, 2024

An Issue I encounter with that approach is that the juliaupselfbin which would be the parent folder to get_juliaup_path(), is locked behind the selfupdate feature, and so, not available in all places where access to the self path is required (i.e.: in the install_version function in the operations.rs file).

Another issue is that that would require passing the GlobalPaths object to many functions that as of now do not require it and would need to get their signature updated:

  • install_background_selfupdate(interval: i64)
  • get_bin_dir()
  • do_initial_setup(juliaupconfig_path: &Path)
  • run_versiondb_update( config_file: &juliaup::config_file::JuliaupReadonlyConfigFile, )
  • run_selfupdate(config_file: &juliaup::config_file::JuliaupReadonlyConfigFile)

are all functions that would need to change signature for them to use the GlobalPaths object.

Now that I dig a bit more in depth, I noticed also some inconsistencies on how the program refers to its own bin folder:
there is in use the functionget_bin_dir() (from #165) for some symlink operations, which searches an env variable JULIAUP_BIN_DIR or defaults to the parent of the binary if and only if the bin dir is within the user home folder.
But in the GlobalPaths object we refer to the bin folder only as the parent of the executable (it was also like that before my changes).

It would very much be convenient to standardise this, so that there is only one "authority" that tells us which is the bin folder.
From my point of view there are three possibilities:

  • let the binary (current_exe()) decide where is the bin folder, but that would conflict with existing JULIAUP_BIN_DIR and therefore would require either rethinking that pull request. Moreover, now that JULIAUP_DEPOT_PATH is a thing, we would have that juliaupselfhome != juliauphome which I have trouble imagining a scenario where that could be beneficial.
  • let the bin dir (either JULIAUP_BIN_DIR or a default location) decide where to find the executable. This in my opinion would instead create some possible conflicts with the JULIAUP_DEPOT_PATH which would need to remain in sync ot it might create inconsistencies.
  • let the JULIAUP_DEPOT_PATH (or a default path if it isn't set) be the only authority where we can find the bin dir and therefore the binary. This means getting rid of both JULIAUP_BIN_DIR and current_exe() calls.

Personally I think that the most robust option is using JULIAUP_DEPOT_PATH as the only authority if juliaup get's installed in a non default location. It makes little sense to have both JULIAUP_DEPOT_PATH and JULIAUP_BIN_DIR, and at that point we could also get rid of all current_exe() calls.
then

  • juliaupselfhome = $JULIAUP_DEPOT_PATH
  • juliaupselfbin = juliaupselfhome\bin
  • my_own_path = juliaupselfbin\juliaup

But it could be breaking for some (probably only those using JULIAUP_BIN_DIR, which isn't even documented).

Unless I am missing something foundamental, I don't really see why we should ever have juliauphome != juliaupselfhome (which would require keeping the reference to the current_exe)

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.

juliaup doesn't correctly resolve symlinks when self updating on macOS
2 participants