Skip to content

feature: confirm on the first time using quickinstall #2223

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

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

NobodyXu
Copy link
Member

@NobodyXu NobodyXu commented Jul 5, 2025

Fixed #2213

if the quickinstall stats url has changed, it'd also require confirm

@NobodyXu NobodyXu changed the title feature: confirm feature: confirm when using sending telemetry to quickinstall Jul 5, 2025
@NobodyXu NobodyXu changed the title feature: confirm when using sending telemetry to quickinstall feature: confirm on the first time using quickinstall Jul 5, 2025
@passcod
Copy link
Member

passcod commented Jul 5, 2025

Hmm, two thoughts:

  1. perhaps we should instead have a config file, like binstall.toml, and for now just have that single item there
  2. should we allow saying no, and in this case disabling the telemetry? so the choice becomes one of:
  • not asked yet
  • url has changed and needs a reconfirm
  • okay with telemetry
  • not okay with telemetry

so maybe something like

[telemetry]
enable = true
url = "https://..."

and then we kinda need to think about / decide what happens if the user changes the url. should we respect that? or should we prompt to change it?

@NobodyXu
Copy link
Member Author

NobodyXu commented Jul 5, 2025

Yeah I think a formal style is better, though I don't think it should be user configured?

I was thinking something like our .json manifest file

@passcod
Copy link
Member

passcod commented Jul 5, 2025

I don't think it should be user configured either but I think if it's text (whether that's toml or json), someone will edit it, so we should probably decide what to do in that case.

@NobodyXu
Copy link
Member Author

NobodyXu commented Jul 5, 2025

Yeah using toml is definitely more readable than just a string

NobodyXu and others added 8 commits July 16, 2025 20:25
Signed-off-by: Jiahao XU <[email protected]>
Signed-off-by: Jiahao XU <[email protected]>
Signed-off-by: Jiahao XU <[email protected]>
Signed-off-by: Jiahao XU <[email protected]>
Signed-off-by: Jiahao XU <[email protected]>
Signed-off-by: Jiahao XU <[email protected]>
Co-authored-by: Félix Saparelli <[email protected]>
Signed-off-by: Jiahao XU <[email protected]>
@NobodyXu NobodyXu force-pushed the feat/quickinstall-confirmation branch from 8b36bf6 to 87227bb Compare July 16, 2025 10:25
Signed-off-by: Jiahao XU <[email protected]>
@NobodyXu
Copy link
Member Author

Hello @passcod been a bit busy recently to finish this off, do you have time to work on this?

@passcod
Copy link
Member

passcod commented Jul 16, 2025

I'll put it on my list for this weekend

@passcod passcod force-pushed the feat/quickinstall-confirmation branch from be4da61 to 0a2bacf Compare July 19, 2025 18:07
@passcod passcod requested review from passcod and removed request for passcod July 19, 2025 18:08
@passcod
Copy link
Member

passcod commented Jul 19, 2025

@NobodyXu I think I've got this in a good place

@passcod passcod force-pushed the feat/quickinstall-confirmation branch from 0a2bacf to 1bf599e Compare July 19, 2025 18:11
let cargo_config = if let Some(config) = cargo_config {
config
} else {
CargoConfig::load_from_path(cargo_root.join("config.toml"))?
Copy link
Member Author

Choose a reason for hiding this comment

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

There's a catch here: what if the new config loaded does specify a install path?

We probably want to load the config before getting cargo_root

Comment on lines +59 to +64
let settings_path = args.settings.clone().unwrap_or(
cargo_home
.as_ref()
.unwrap_or(&cargo_root)
.join("binstall.toml"),
);
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
let settings_path = args.settings.clone().unwrap_or(
cargo_home
.as_ref()
.unwrap_or(&cargo_root)
.join("binstall.toml"),
);
let settings_path = args.settings.as_deref().map(Cow::Borrowed).unwrap_or_else(||
Cow::Owned(cargo_home
.as_ref()
.unwrap_or(&cargo_root)
.join("binstall.toml")
)
);

} else if let Some(p) = &settings.install_path {
debug!(path=?p, "install path from settings");
(p.into(), true)
} else if cargo_home.is_some() {
Copy link
Member Author

Choose a reason for hiding this comment

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

What even if cargo_root is set, we won't use it if cargo_root is set?

.unwrap_or(&cargo_root)
.join("binstall.toml"),
);
let mut settings = crate::settings::load(args.settings.is_some(), &settings_path)?;
Copy link
Member Author

Choose a reason for hiding this comment

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

If we ignore the error when args not set, then we might still fail at installation, e.g. the settings is occupied by a directory/special file, or no permissions

Comment on lines +14 to +35
pub struct Settings {
#[serde(default = "tru")]
pub confirm: bool,

#[serde(default)]
pub install_path: Option<PathBuf>,

#[serde(default = "tru")]
pub track_installs: bool,

#[serde(default)]
pub continue_on_failure: bool,

#[serde(default)]
pub targets: Option<Vec<String>>,

#[serde(default)]
pub strategies: Vec<StrategyWrapped>,

#[serde(default)]
pub telemetry: Telemetry,
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm personally I think putting it in binstalk-manifest makes more sense

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.

cargo-binstall calling home to fly.io instead of site stated in README.
2 participants