Detect Elan self update disabled (e.g. installed from distro repos)

[grahnen]

What does this PR do

Adds a config option to disable elan self-update (for people like me, who installed elan from distro repos).

Standards checklist

• The PR title is descriptive
• I have read CONTRIBUTING.md
• Optional: I have tested the code myself
• If this PR introduces new user-facing messages they are translated

For new steps

• Optional: Topgrade skips this step where needed
• Optional: The --dry-run option works with this step
• Optional: The --yes option works with this step if it is supported by
  the underlying command

If you developed a feature or a bug fix for someone else and you do not have the
means to test it, please tag this person here.

[SteveLauC]

who installed elan from distro repos

Is there any way we can know this? If so, we can automatically skip this rather than hardcoding it? Does elan self update give any error in this case?

[grahnen]

who installed elan from distro repos

Is there any way we can know this? If so, we can automatically skip this rather than hardcoding it? Does elan self update give any error in this case?

It prints an error message with the reason, but returns with error code 1, just like any other error.

[SteveLauC]

It prints an error message with the reason, but returns with error code 1, just like any other error.

Great, then we can use it! Just like what we did here: https://github.com/topgrade-rs/topgrade/pull/971/files

[grahnen]

It prints an error message with the reason, but returns with error code 1, just like any other error.

Great, then we can use it! Just like what we did here: https://github.com/topgrade-rs/topgrade/pull/971/files

There is no error when elan self --help is called; only when elan self update is called.

I reused the code for Helm, which checks if the error message contains this specific error, and in such a case just keeps going with the rest of the update.

It still prints the error, which may or may not be desired behaviour.

[SteveLauC]

It still prints the error, which may or may not be desired behaviour.

If you use output_checked() directly, then the error won't be printed.

Can you just do, I guess this would work

let result = ctx
        .run_type()
        .execute(&uv_exec)
        .args(["self", "update"])
        .output_checked();
if result.is_ok() {
    ctx.run_type()
        .execute(&uv_exec)
        .args(["self", "update"])
        .status_checked()?;
}

[grahnen]

This would do self-update twice, if the first one succeeds, right?
Is that better than comparing the output of the "self update" call to a saved error message?

[SteveLauC]

This would do self-update twice, if the first one succeeds, right?

Right, thanks for catching this. I was probably dizzy at that time 🤦‍♂️

---

Is that better than comparing the output of the "self update" call to a saved error message?

I think we do need to compare the output with the saved message, what about:

1. Do elan self update, capture the output
   1. If 1 succeeds, the self-update feature is available and it just successfully performed the update, we print!() the captured stdout and eprint!() the captured stderr
   2. If 1 fails, there are possibly 2 cases why this failure happened:
      1. The self-update feature is unavailable (stdout or stderr contains the saved error message), nothing further needs to be done
      2. The self-update feature is available, but the operation just failed, error out
2. Do elan update

    // If elan is externally managed, then running `elan self update` will give an 
    // error message that contains `disabled_error_msg`, in this case, Topgrade 
    // tries to hide that `elan self update` has just been executed.
    let disabled_error_msg = "self-update is disabled";
    let executor_output = ctx.run_type().execute(&elan).args(["self", "update"]).output()?;
    match executor_output{
        ExecutorOutput::Wet(command_output) => {
            if command_output.status.success() {
                // Flush the captured output
                std::io::stdout().lock().write(&command_output.stdout).unwrap();
                std::io::stderr().lock().write(&command_output.stderr).unwrap();
            } else {
                let stderr_as_str = std::str::from_utf8(&command_output.stderr).unwrap();
                if stderr_as_str.contains(disabled_error_msg) {
                    // `elan` is externally managed, we cannot do the update. Users
                    // won't see any error message because Topgrade captures them
                    // all.
                } else {
                    // `elan` is NOT externally managed, `elan self update` can 
                    // be performed, but the invocation failed, so we report the
                    // error to the user and error out.
                    std::io::stdout().lock().write(&command_output.stdout).unwrap();
                    std::io::stderr().lock().write(&command_output.stderr).unwrap();

                    return Err(StepFailed.into());
                }
            }

        }
        ExecutorOutput::Dry => {/* nothing needed because in a dry run */}
    }

[grahnen]

The latest commit hides the output if the "self update" command fails because it is disabled, otherwise returns the error of that command. This, to me, seems better than running "self update" twice. Let me know if you disagree.

[SteveLauC]

The latest commit hides the output if the "self update" command fails because it is disabled, otherwise returns the error of that command. This, to me, seems better than running "self update" twice. Let me know if you disagree.

Looks like we are doing the same thing, check out my last comment, it provides an implementation with slightly better UX

[SteveLauC]

Though my patch needs this #1002 to work

[grahnen]

The latest commit hides the output if the "self update" command fails because it is disabled, otherwise returns the error of that command. This, to me, seems better than running "self update" twice. Let me know if you disagree.

Looks like we are doing the same thing, check out my last comment, it provides an implementation with slightly better UX

Sorry, I missed that one. Yes, we are doing the same thing, and I agree yours do look better code-wise. Let's wait for #1002, and fix this once that is merged.

The "filtering error messages" methodology might be common enough to create a function
output_checked_or (similar to output_checked_with but filtering errors rather than successes).

[SteveLauC]

The "filtering error messages" methodology might be common enough to create a function
output_checked_or (similar to output_checked_with but filtering errors rather than successes).

If it becomes commonly used, yeah, we should make it a utility

[SteveLauC]

Let's wait for #1002, and fix this once that is merged.

#1002 merged

[grahnen]

I took your code and adjusted it with clippy's suggestions. It should be ready now :)

[SteveLauC]

Thanks