Prompt user to install the override toolchain if its specified but not installed yet #656

alfiedotwtf · 2024-08-26T18:11:54Z

This PR is in relation to #652

When overriding toolchains, there are 2 places to be concerned with when a toolchain is missing:

When fuelup is the command run
When a proxy command is run

For 2, we are already covering this scenario by installing missing components and plugins on first run.

As for 1, this PR will now bail!() unless the command is to install, uninstall, or create a new toolkit.

...

No tests yet because I want to get feedback to see if I'm on the right track.

fuel-service-user · 2024-09-02T14:45:40Z

LCOV of commit `69176e8` during CI #2038

Summary coverage rate:
  lines......: 82.3% (2364 of 2873 lines)
  functions..: 44.6% (367 of 823 functions)
  branches...: 59.6% (272 of 456 branches)

Files changed coverage rate: n/a

alfiedotwtf · 2024-09-02T15:03:35Z

Once I found that toolchain installations were not failing because of code, everything started falling into place...

I've left the dialoguer comment in fmt.rs so that you can all see how that library is not able to be tested without a real tty (which is a shame given it works really nicely). So as is, we are still currently prompting the user via the original hand-rolled stdin nibbler.

We should now be a bit more robust with this PR when parsing <channel>-<date>-<target>, however I have left out most of the split on "-" code which I believe is/was causing most of the problems when specifying channel/toolchains, because it may be breaking behaviour which I'll avoid for now.

But for something to keep in mind, in the future we're probably better off converting split on "-" to regexes.

alfiedotwtf · 2024-09-02T22:09:10Z

I forgot to mention, with the toolchain spec here:

/// Parses a distributable toolchain description from a string.
///
/// The supported formats are:
///     <channel>
///     <channel>-<target>
///     <channel>-<YYYY-MM-DD>
///     <channel>-<YYYY-MM-DD>-<target>
///     <channel>-<target>-<YYYY-MM-DD>
/// The parsing begins from the end of the string, so the target is the last part of the string,
/// then the date and finally the name
impl FromStr for DistToolchainDescription {

I think <channel>-<target>-<YYYY-MM-DD> should be removed since it doesn't follow the channel format of <channel>[-<date>].

--

Other quick change to the PR could be to move the URL concats into a separate function like fn github_releases_download_url(), but figured it was easy enough to leave it as is. Happy to change it if someone thinks it's better to go that way.

sdankel

It's coming along! I left some comments, and make sure to run cargo fmt to get CI passing.

sdankel · 2024-09-04T17:05:09Z

src/channel.rs

@@ -68,7 +50,7 @@ fn format_nightly_url(date: &Date) -> Result<String> {
 }

 fn construct_channel_url(desc: &DistToolchainDescription) -> Result<String> {
-    let mut url = FUELUP_GH_PAGES.to_owned();
+    let mut url = format!("{}{}/gh-pages/", GITHUB_USER_CONTENT_URL, "fuelup");


I'd prefer to keep FUELUP_GH_PAGES as a constant and refactor this to not use a mutable url.

fn construct_channel_url(desc: &DistToolchainDescription) -> Result<String> { let channel_file = match desc.name { DistToolchainName::Latest => { if let Some(date) = desc.date { &format!("channels/latest/channel-fuel-latest-{date}.toml") } else { CHANNEL_LATEST_FILE_NAME } } DistToolchainName::Nightly => { let mut file = "".to_string(); if let Some(date) = desc.date { file.push_str(&format_nightly_url(&date)?); file.push('/'); } file.push_str(CHANNEL_NIGHTLY_FILE_NAME); &file } DistToolchainName::Beta1 => CHANNEL_BETA_1_FILE_NAME, DistToolchainName::Beta2 => CHANNEL_BETA_2_FILE_NAME, DistToolchainName::Beta3 => CHANNEL_BETA_3_FILE_NAME, DistToolchainName::Beta4 => CHANNEL_BETA_4_FILE_NAME, DistToolchainName::Beta5 => CHANNEL_BETA_5_FILE_NAME, DistToolchainName::Devnet => CHANNEL_DEVNET_FILE_NAME, DistToolchainName::Testnet => CHANNEL_TESTNET_FILE_NAME, }; let mut url = format!("{}{}/gh-pages/", GITHUB_USER_CONTENT_URL, "fuelup"); Ok(format!("{FUELUP_GH_PAGES}{channel_file}")) }

sdankel · 2024-09-04T17:06:06Z

src/channel.rs

+pub fn is_beta_toolchain(name: &str) -> Option<&str> {
+    BETA_CHANNELS
+        .iter()
+        .find(|&beta_channel| name.starts_with(beta_channel))


Shouldn't this still be name == beta_channel?

sdankel · 2024-09-04T17:07:40Z

src/commands/toolchain.rs

@@ -45,7 +45,7 @@ fn name_allowed(s: &str) -> Result<String> {
        None => s,
    };

-    if RESERVED_TOOLCHAIN_NAMES.contains(&name) {
+    if CHANNELS.contains(&name) {


RESERVED_TOOLCHAIN_NAMES is different from CHANNELS. We want stable to be reserved even though it isn't currently a usable channel.

sdankel · 2024-09-04T17:07:58Z

src/config.rs

@@ -25,7 +25,7 @@ impl Config {
                .filter(|e| e.file_type().map(|f| f.is_dir()).unwrap_or(false))
            {
                let toolchain = dir_entry.file_name().to_string_lossy().to_string();
-                if RESERVED_TOOLCHAIN_NAMES
+                if CHANNELS


Same comment here.

sdankel · 2024-09-04T17:13:21Z

src/fmt.rs

@@ -57,4 +57,14 @@ pub fn ask_user_yes_no_question(question: &str) -> io::Result<bool> {
            return Ok(result);
        }
    }
+
+    // This is the dialoguer version of the prompter, but it's not testable


Have you tried testing it with rexpect? Here's an example

sdankel · 2024-09-04T17:15:01Z

src/fuelup_cli.rs

@@ -46,6 +51,41 @@ enum Commands {
 pub fn fuelup_cli() -> Result<()> {
    let cli = Cli::parse();

+    if let Some(toolchain_override) = ToolchainOverride::from_project_root() {


nit: add a comment here explaining what this is checking

sdankel · 2024-09-04T17:22:03Z

src/fuelup_cli.rs

+
+        if !toolchain.exists() {
+            match cli.command {
+                Commands::Toolchain(_) => {


We only want to do this for operations where the toolchain override will be used in some way, right? So we also don't need to do this for fuelup update/upgrade/install fuelup completions fuelup check fuelup default

We really only need it for proxy commands and for fuelup component which modifies the currently active toolchain.

For fuelup show it could be good to show the current state first and then prompt the user if they wish to install.

sdankel · 2024-09-04T17:23:03Z

tests/default.rs

+
+        assert!(output
+            .stdout
+            .starts_with(&format!("Using override toolchain 'beta-1-{triple}'")));


nit: we're planning to remove support for betas 1-4 pretty soon, so it might be better to make these tests use beta-5

sdankel · 2024-09-04T17:24:54Z

tests/testcfg/mod.rs

+
+    /// A convenience wrapper for executing 'fuelup' within the fuelup test configuration.
+    /// It takes an array ref of bytes to write into stdin.
+    pub fn fuelup_with_input(&mut self, args: &[&str], input: &[u8]) -> TestOutput {


This should be easier with rexpect

alfiedotwtf · 2024-11-14T02:47:23Z

Keeping this PR in DRAFT but backgrounding for now...

Most of these changes have moved to other PRs and have been merged, but there's a few change and comments remaining that I want to pull into future changes.

alfiedotwtf requested review from JoshuaBatty, kayagokalp and sdankel August 26, 2024 18:12

alfiedotwtf self-assigned this Aug 26, 2024

alfiedotwtf added help wanted Extra attention is needed question Further information is requested fuelup labels Aug 26, 2024

alfiedotwtf added 3 commits September 3, 2024 00:31

Factor out repeated code and data (#652)

bcdb5f8

Fix beta checking from exact match to prefix (#652)

b4d9116

Prompt user to install toolchain if missing (#652)

69176e8

alfiedotwtf force-pushed the alfie/652-bail-when-override-missing branch from 1f2086b to 69176e8 Compare September 2, 2024 14:42

alfiedotwtf marked this pull request as ready for review September 2, 2024 14:42

alfiedotwtf changed the title ~~Bail when override toolchain is not installed~~ Prompt the user to install the override toolchain if its specified but not installed yet Sep 2, 2024

alfiedotwtf changed the title ~~Prompt the user to install the override toolchain if its specified but not installed yet~~ Prompt user to install the override toolchain if its specified but not installed yet Sep 2, 2024

sdankel requested changes Sep 4, 2024

View reviewed changes

alfiedotwtf marked this pull request as draft September 10, 2024 08:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prompt user to install the override toolchain if its specified but not installed yet #656

Prompt user to install the override toolchain if its specified but not installed yet #656

alfiedotwtf commented Aug 26, 2024 •

edited

Loading

fuel-service-user commented Sep 2, 2024

alfiedotwtf commented Sep 2, 2024

alfiedotwtf commented Sep 2, 2024

sdankel left a comment

sdankel Sep 4, 2024

sdankel Sep 4, 2024

sdankel Sep 4, 2024

sdankel Sep 4, 2024 •

edited

Loading

sdankel Sep 4, 2024

sdankel Sep 4, 2024

sdankel Sep 4, 2024

sdankel Sep 4, 2024

sdankel Sep 4, 2024

alfiedotwtf commented Nov 14, 2024

Prompt user to install the override toolchain if its specified but not installed yet #656

Are you sure you want to change the base?

Prompt user to install the override toolchain if its specified but not installed yet #656

Conversation

alfiedotwtf commented Aug 26, 2024 • edited Loading

fuel-service-user commented Sep 2, 2024

LCOV of commit 69176e8 during CI #2038

alfiedotwtf commented Sep 2, 2024

alfiedotwtf commented Sep 2, 2024

sdankel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdankel Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alfiedotwtf commented Nov 14, 2024

alfiedotwtf commented Aug 26, 2024 •

edited

Loading

LCOV of commit `69176e8` during CI #2038

sdankel Sep 4, 2024 •

edited

Loading