-
Notifications
You must be signed in to change notification settings - Fork 95
Offer to / automatically install versions via channel selection #1218
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Instead of using sorted_by() with an iterator chain, which can cause sorting panics with unstable comparison functions, this now: Collects all the database channels into a vector Extending it with the non-database channels Uses sort_by() to sort the entire collection in place This should resolve the i686 Windows CI failure because: sort_by() is more robust than sorted_by() for handling comparison functions that might not provide perfect total ordering All channels are sorted together rather than having separate sorted and unsorted sections The sorting happens on the complete dataset rather than being chained
@davidanthoff I would prefer to be able to force push tidying these commits up into meaningful commits, but force push remains disallowed here. I think it should only be disabled for |
@StefanKarpinski FYI. I think you mentioned this idea somewhere. |
Cool, I really like this feature! A few comments:
Implementation wise, it might be the best strategy to separate the interactive mode detection code from #1095 as a separate PR, merge that, and then my first point above could probably be implemented fairly quickly? I hear you on the push restriction, but without it things happened in the past where I ended up literally spending hours trying to fix push/merge conflicts and I really don't want to deal with that :) The history here is a total mess in any case already, so there is no point in putting time into a nice history for a PR, it would be a diamond in a very rough sea ;) |
UI updated under my mouse.. accidental close. @davidanthoff sounds good. I've made some changes along those lines When a channel isn't installed, users now get prompted:
In non-interactive mode (no TTY), it defaults to "no" and shows the standard error message. The config is 3-state:
And the launcher now spawns |
src/bin/julialauncher.rs
Outdated
@@ -132,6 +134,119 @@ fn run_selfupdate(_config_file: &juliaup::config_file::JuliaupReadonlyConfigFile | |||
Ok(()) | |||
} | |||
|
|||
fn is_interactive() -> bool { | |||
std::io::stdin().is_terminal() && std::io::stderr().is_terminal() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a necessary condition, but I think in addition we need to parse the command line arguments and figure out whether the Julia process will jump into the REPL or execute either a script/code snippet. I think generally that if someone runs julia +FOO main.jl
we should not show anything interactive, regardless of whether things are running in a terminal or not.
I think the PR I linked to above should have code that does that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't find that check in the linked PR, but added this check based on julia args.
src/bin/julialauncher.rs
Outdated
std::io::stderr().flush()?; | ||
|
||
let mut input = String::new(); | ||
std::io::stdin().read_line(&mut input)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already use https://github.com/console-rs/dialoguer in the installer, we could use it here as well and have a consistent UI? But I'm not sure whether it actually has the kind of one-key-press option that I think would be ideal here.
I'm happy to merge as is if you want to keep it that way, just wanted to point to it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that's nicer. I wasn't aware. Updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, that was fast :)
The only thing that I think we need to address is the interactive detection, otherwise looks great.
Oh, and I should say: Once we have that, we should do a similar thing for the message that a channel is out-of-date, it also should just show a prompt offering to update right away if we are in interactive mode! |
This comment was marked as resolved.
This comment was marked as resolved.
Bump @davidanthoff Also,
I've been thinking about this and I don't think we should. I very often don't want to update because I just want to test what I have (and not hit more precompilation). I think adding a prompt in there might be more annoying than helpful. |
Yes, please no prompts. |
@davidanthoff bump. To try and help this along I'll see what copilot makes of it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements automatic installation of Julia channels when they are requested via command line channel selection (e.g., julia +1.8.2
). It adds a new configuration option autoinstallchannels
that controls whether to automatically install missing but valid channels.
- Adds
autoinstallchannels
configuration option with true/false/default values - Implements automatic installation of valid channels when requested via
+channel
syntax - Updates test suite to verify auto-installation behavior works correctly
Reviewed Changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
src/command_config_autoinstall.rs | New module implementing the autoinstallchannels configuration command |
src/config_file.rs | Adds auto_install_channels field to configuration structure with proper serialization |
src/cli.rs | Adds AutoInstallChannels subcommand to the config CLI interface |
src/lib.rs | Exports the new command_config_autoinstall module |
src/operations.rs | Fixes missing win32 handling and improves error message for pull request downloads |
tests/channel_selection.rs | Updates existing tests and adds new test for auto-installation functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Makes it such that if you specify a channel which you don't have installed but is valid, it installs and launches it, with a message.
Works for all valid channel descriptors (including PRs).
Developed with Claude.
Note unrelated bugfixes that are included:
7f7e956
(#1218) - fix: add missing win32 handling in install_non_db_versionc6eafe3
(#1218) - fix win32 sorting panicIf this PR isn't a simple merge it might be worth separating those out as a bugfix.