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

automatically create symlinks for channels #165

Merged
merged 2 commits into from
Dec 28, 2021
Merged

Conversation

simeonschaub
Copy link
Member

With this PR, Juliaup automatically creates symlinks in ~/.local/bin
for each new channel and updates them accordingly. I have intentionally
disabled this on Windows for now since symlink is not supported there
and I am not sure if there even is a PATH equivalent on Windows.

This functionality follows other tools such as jill and also allows
users to avoid going through julialauncher, which can sometimes
complicate things, e.g. when running Julia through GDB.

One remaining question is whether we want to add an option or env var to
turn off this behavior.

@simeonschaub simeonschaub added the enhancement New feature or request label Nov 22, 2021
@simeonschaub simeonschaub force-pushed the sds/symlinks branch 2 times, most recently from 821a401 to ae6bb5c Compare November 22, 2021 04:44
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.

Ha, I've been thinking about that :) I'll start with the things I don't like about this option, BUT I should say that I think in the end the benefits outweigh these downsides.

Things I don't like:

  • There is more stuff in various places left around than in the simple version, so a complete uninstall of juliaup and julia becomes more involved difficult.
  • There are more config things that can go wrong.
  • I don't like that there is a difference between platforms. I think that in principle we won't be able to do this on Windows, at least not for the store distributed version.

But I think at the end of the day this is just too good of a feature and we should probably just do it :)

I do like the idea that we make this a configurable feature, maybe off by default. We probably need to think about configuration in general, but I could imagine commands like juliaup config symlinks on and juliaup config symlinks off or something like that. And when the configuration value changes, juliaup could automatically delete all symlinks, or create symlinks for all channels.

I think we need to coordinate a bit with how the shell install script that I merged today works, right? I think it actually no longer creates symlinks because at the end of the day there is no one location that is user specific that is on the PATH, so instead it now actually adds the folder where juliaup is installed to the PATH. I think if we stick with that, then maybe this PR here could actually create all the symlinks in the same folder? That would mitigate the problem of having stuff spread out on the system quite a bit? Plus, it would just align better with how juliaup is installed now. So in the case of the shell script installed story I think that would be the best option, but then we also have scenarios of course where we hope that juliaup is deployed by a system package manager, like on arch, and then I'm not sure whether we need yet another strategy there?

I think this here also creates a symlink for the default julia, right? We should probably not do that, as that command is already handled by the launcher, plus users would then expect that things like +version work on that. And eventually I think we will want to add support for automatically starting the Julia version in a Manifest.toml etc.

Another question is how this interacts with the JULIA_DEPOT env variable... juliaup at the moment works well with that, not clear how that would work here...

Ok, enough for now :) I think generally I'm on board with this, but we probably just need to tinker around a bit more and think some more scenarios through, I'd say?

match config_data.installed_channels.get(&channel).unwrap() {
JuliaupConfigChannel::SystemChannel { version } =>
create_symlink(&version, &"julia".to_string())?,
_ => eprintln!("Symlink creation currently not supported for linked channels."),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason for that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since linked channels are specified by a system command as well as command line arguments, we can't just create a symlink, we would probably want to create a small shell script that just calls that command and passes on any additional arguments.

@@ -136,3 +135,67 @@ pub fn garbage_collect_versions(config_data: &mut JuliaupConfig) -> Result<()> {

Ok(())
}

fn _remove_symlink(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could add the conditional compile, right?

Ok(())
}

pub fn remove_symlink(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could add the conditional compile, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was a bit unsure how to best handle this in rust. With the current implementation I believe this would still need some shims for the windows case, since otherwise rust complains during compilation that the function is not defined. Maybe there is a straightforward way to statically tell rust that this branch never gets taken on windows though.

@simeonschaub
Copy link
Member Author

I do like the idea that we make this a configurable feature, maybe off by default. We probably need to think about configuration in general, but I could imagine commands like juliaup config symlinks on and juliaup config symlinks off or something like that. And when the configuration value changes, juliaup could automatically delete all symlinks, or create symlinks for all channels.

Yes, that sounds reasonable.

I think we need to coordinate a bit with how the shell install script that I merged today works, right? I think it actually no longer creates symlinks because at the end of the day there is no one location that is user specific that is on the PATH, so instead it now actually adds the folder where juliaup is installed to the PATH. I think if we stick with that, then maybe this PR here could actually create all the symlinks in the same folder? That would mitigate the problem of having stuff spread out on the system quite a bit? Plus, it would just align better with how juliaup is installed now. So in the case of the shell script installed story I think that would be the best option, but then we also have scenarios of course where we hope that juliaup is deployed by a system package manager, like on arch, and then I'm not sure whether we need yet another strategy there?

Ah, I haven't looked at that PR in detail yet. That seems like a good option, but supporting every scenario probably requires some careful thought.

I think this here also creates a symlink for the default julia, right? We should probably not do that, as that command is already handled by the launcher, plus users would then expect that things like +version work on that. And eventually I think we will want to add support for automatically starting the Julia version in a Manifest.toml etc.

Yes, I didn't think about that, I agree that was probably a bad idea.

Another question is how this interacts with the JULIA_DEPOT env variable... juliaup at the moment works well with that, not clear how that would work here...

It respects the JULIA_DEPOT variable when creating the symlink. The only issue would be if the depot moved at some point, since the symlinks would still be pointing to the old location.

@simeonschaub
Copy link
Member Author

Ok, I have updated this PR according to your comments. There is now a create_symlinks option in the juliaup.json file which defaults to false and can be set using juliaup config symlinks on/off. Changing this option will also add or remove symlinks for every installed channel accordingly.

I have also changed the default directory for the symlinks from ~/.local/bin to the directory of the juliaup executable. It can still be changed by setting the JULIAUP_BIN_DIR env var however.

@simeonschaub
Copy link
Member Author

So in the case of the shell script installed story I think that would be the best option, but then we also have scenarios of course where we hope that juliaup is deployed by a system package manager, like on arch, and then I'm not sure whether we need yet another strategy there?

Yes I think we probably want to think about that case as well, since currently on arch juliaup is installed into the system directory, so you would need superuser privilege to create symlinks there. Can we rely on the fact that if JuliaUpChannel is None, juliaup was installed via another package manager and we should create symlinks in ~/.local/bin instead?

@simeonschaub simeonschaub changed the title RFC: automatically create symlinks for channels automatically create symlinks for channels Dec 3, 2021
@simeonschaub simeonschaub marked this pull request as ready for review December 3, 2021 01:02
With this PR, Juliaup automatically creates symlinks in `~/.local/bin`
for each new channel and updates them accordingly. I have intentionally
disabled this on Windows for now since `symlink` is not supported there
and I am not sure if there even is a `PATH` equivalent on Windows.

This functionality follows other tools such as jill and also allows
users to avoid going through `julialauncher`, which can sometimes
complicate things, e.g. when running Julia through GDB.

One remaining question is whether we want to add an option or env var to
turn off this behavior.
@davidanthoff davidanthoff merged commit eeba964 into master Dec 28, 2021
@davidanthoff davidanthoff deleted the sds/symlinks branch December 28, 2021 02:28
@davidanthoff
Copy link
Collaborator

Alright, I merged, as I think this is fantastic, plus I want the configuration infrastructure on master :)

I think we might want to tweak a few things in follow up PRs:

  • Not sure about the on/off in the command line. Maybe we just use true and false there as well?
  • We can make that property in the configuration optional, so that it doesn't get set at all, maybe?

Can we rely on the fact that if JuliaUpChannel is None, juliaup was installed via another package manager and we should create symlinks in ~/.local/bin instead

Hm, not sure, maybe? Or maybe we just handle this via conditional compiles? Not entirely sure what the best strategy here is...

@simeonschaub
Copy link
Member Author

  • Not sure about the on/off in the command line. Maybe we just use true and false there as well?

Yeah, I just copied your original proposal one-to-one, but I agree that true/false might be a more standard name. If you are working on the config infrastructure anyways, feel free to change it.

  • We can make that property in the configuration optional, so that it doesn't get set at all, maybe?

I thought that's what the default option in serde did, but that might only be relevant for reading, not for writing properties.

Can we rely on the fact that if JuliaUpChannel is None, juliaup was installed via another package manager and we should create symlinks in ~/.local/bin instead

Hm, not sure, maybe? Or maybe we just handle this via conditional compiles? Not entirely sure what the best strategy here is...

The logic now is that get_bin_dir checks if the Juliaup binary is inside the home directory and if it is then it uses that directory for the symlinks as well, otherwise it uses ~/.local/bin. I think that's probably reasonable for now, in the future we might want to think about something more self-contained though. It can also still be customized using an env var.

@davidanthoff
Copy link
Collaborator

The logic now is [...]

Yes, agreed, that seems pretty robust!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants