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

Fix fetcher::QuickInstallstats report sending #918

Merged
merged 3 commits into from
Mar 17, 2023
Merged

Conversation

NobodyXu
Copy link
Member

@NobodyXu NobodyXu commented Mar 17, 2023

Fixed cargo-bins/cargo-quickinstall#195

  • Fix fetchers::QuickInstall: Stop sending stats for universal-apple-darwin
    since quickinstall only supports targets officially supports by rust.

  • Only send stats report to quickinstall if the Fetcher::find is .awaited on

    This prevents stats report to be sent for cases where the QuickInstall fetcher
    is actually unused, e.g. resolved to GhCrateMeta or other QuickInstall fetcher
    with different target.

    This also reduces amount of http requests created in background.

Signed-off-by: Jiahao XU Jiahao_XU@outlook.com

…darwin

since quickinstall only supports targets officially supports by rust.

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
…ait`ed on

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu NobodyXu requested a review from passcod March 17, 2023 08:16
@passcod
Copy link
Member

passcod commented Mar 17, 2023

Does the latter point still send reports when installing from source?

@passcod
Copy link
Member

passcod commented Mar 17, 2023

also while thinking of it, we should support universal2-apple-darwin, which is the convention for arm+intel in another ecosystem, iirc python (to differentiate with the earlier universal for powerpc+intel)

@NobodyXu
Copy link
Member Author

Does the latter point still send reports when installing from source?

If quickinstall is not disabled by strategy, then yes, every fetcher::QuickInstall will send report when .awaited on the Fetcher::find return value.

@NobodyXu
Copy link
Member Author

also while thinking of it, we should support universal2-apple-darwin, which is the convention for arm+intel in another ecosystem, iirc python (to differentiate with the earlier universal for powerpc+intel)

Does that mean we need to rename universal-apple-darwin => universal2-apple-darwin, causing existing build artifacts to stop working?

Or do we just create a new alias?

@passcod
Copy link
Member

passcod commented Mar 17, 2023

see axodotdev/cargo-dist#77

@NobodyXu NobodyXu merged commit cc78ff3 into main Mar 17, 2023
@NobodyXu NobodyXu deleted the fix/quickinstall branch March 17, 2023 10:05
@passcod
Copy link
Member

passcod commented Mar 17, 2023

Just a new alias, it's only convention anyway.

NobodyXu added a commit that referenced this pull request Mar 18, 2023
This bug was reintroduced by #919 and it was fixed by #918

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
NobodyXu added a commit that referenced this pull request Mar 18, 2023
)

This bug was reintroduced by #919 and it was fixed by #918

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
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.

Fail to send report
2 participants