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

Do not download self update if it is the same version #576

Merged
merged 7 commits into from
Feb 21, 2024

Conversation

crodas
Copy link
Contributor

@crodas crodas commented Feb 19, 2024

Fixes #555

After:

image

@crodas crodas self-assigned this Feb 19, 2024
@crodas crodas enabled auto-merge (squash) February 19, 2024 21:47
Copy link
Member

@sdankel sdankel left a comment

Choose a reason for hiding this comment

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

Mostly nits, thanks for doing this!

src/file.rs Outdated
@@ -9,6 +10,21 @@ pub(crate) fn is_executable(file: &Path) -> bool {
file.is_file() && file.metadata().unwrap().permissions().mode() & 0o111 != 0
}

pub(crate) fn get_bin_version(exec_path: &Path) -> Option<Version> {
Copy link
Member

Choose a reason for hiding this comment

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

💟

src/ops/fuelup_self.rs Outdated Show resolved Hide resolved
src/ops/fuelup_show.rs Outdated Show resolved Hide resolved
src/ops/fuelup_show.rs Outdated Show resolved Hide resolved
src/ops/fuelup_show.rs Show resolved Hide resolved
src/toolchain.rs Outdated Show resolved Hide resolved
tests/self.rs Show resolved Hide resolved
@crodas crodas force-pushed the improve-self-update branch 4 times, most recently from 5ac35f5 to 1ddd7ad Compare February 21, 2024 19:16
Copy link
Member

@sdankel sdankel left a comment

Choose a reason for hiding this comment

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

Looking good, just a couple nits 👍


#[cfg(unix)]
pub(crate) fn is_executable(file: &Path) -> bool {
file.is_file() && file.metadata().unwrap().permissions().mode() & 0o111 != 0
}

#[derive(Debug, thiserror::Error)]
pub(crate) enum BinError {
Copy link
Member

Choose a reason for hiding this comment

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

This is much better!

info!("{:>4}- {} - Error getting version string", "", plugin);
}
};
match get_bin_version(plugin_executable) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: we can make it even shorter! This function never returns an error, so doesn't need to return an empty result.

fn check_plugin(plugin_executable: &Path, plugin: &str, latest_version: &Version) {
    let version_or_err = match get_bin_version(plugin_executable) {
        Ok(version) => format_version_comparison(&version, latest_version),
        Err(err) => err.to_string()
    };
    info!("{:>4}- {} - {}", "", plugin, version_or_err);
}

Comment on lines 113 to 130
match get_bin_version(&component_executable) {
Ok(version) => {
info!(
"{:>2}{} - {}",
"",
bold(&component.name),
format_version_comparison(&version, latest_version)
);
}
Err(_) => error!(
"{:>2}{} - Error getting version string",
"",
bold(&component.name)
),
};

Err(e) => {
error!(
"{:>2}{} - Error getting version string: {}",
"",
bold(&component.name),
e.to_string()
);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Same thing here:

            let version_text = match get_bin_version(&component_executable) {
                Ok(version) => format_version_comparison(&version, latest_version),
                Err(err) => err.to_string(),
            };
            info!("{:>2}{} - {}", "", bold(&component.name), version_text);

@crodas crodas merged commit 276ba1e into master Feb 21, 2024
16 checks passed
@crodas crodas deleted the improve-self-update branch February 21, 2024 22:01
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.

fuelup self update downloading the same version
2 participants