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

[Installer Downloader] Detect native CPU arch #7709

Merged
merged 3 commits into from
Feb 27, 2025

Conversation

MarkusPettersson98
Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 commented Feb 21, 2025

This change is Reviewable

@MarkusPettersson98 MarkusPettersson98 force-pushed the detect-native-arch branch 2 times, most recently from 5150145 to 03d081c Compare February 21, 2025 14:47
@MarkusPettersson98 MarkusPettersson98 marked this pull request as ready for review February 21, 2025 15:18
@MarkusPettersson98 MarkusPettersson98 force-pushed the detect-native-arch branch 3 times, most recently from 5ab5ced to be52948 Compare February 21, 2025 16:08
@dlon dlon force-pushed the installer-downloader branch 14 times, most recently from 6994be5 to 4eef94d Compare February 23, 2025 13:26
Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 11 files reviewed, 1 unresolved discussion


installer-downloader/src/controller.rs line 66 at r1 (raw file):

    let Some(architecture) = get_arch().ok().flatten() else {
        // Could not retrieve the host's CPU architecture for whatever reason
        delegate.queue_main(|self_| {

Could the error handling be moved into AppController::initialize? By passing in something like architecture: impl FnOnce() -> io::Result<VersionArchitecture>.

Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

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

Reviewed 10 of 11 files at r1.
Reviewable status: 10 of 11 files reviewed, 2 unresolved discussions (waiting on @MarkusPettersson98)


installer-downloader/Cargo.toml line 19 at r1 (raw file):

[dependencies]
talpid-platform-metadata = { path = "../talpid-platform-metadata" }

This should be gated behind windows || macos.

Copy link
Contributor Author

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 10 of 11 files reviewed, 1 unresolved discussion


installer-downloader/Cargo.toml line 19 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

This should be gated behind windows || macos.

Done

@MarkusPettersson98 MarkusPettersson98 force-pushed the detect-native-arch branch 2 times, most recently from b978d94 to e93e592 Compare February 25, 2025 12:29
@MarkusPettersson98 MarkusPettersson98 marked this pull request as draft February 25, 2025 12:30
@MarkusPettersson98 MarkusPettersson98 force-pushed the detect-native-arch branch 3 times, most recently from 398ab0c to 36fcad9 Compare February 25, 2025 13:27
@MarkusPettersson98 MarkusPettersson98 force-pushed the detect-native-arch branch 2 times, most recently from d83ceee to 9fd34a6 Compare February 25, 2025 13:42
@MarkusPettersson98 MarkusPettersson98 marked this pull request as ready for review February 25, 2025 14:15
@MarkusPettersson98 MarkusPettersson98 force-pushed the detect-native-arch branch 2 times, most recently from 3ab4e11 to bdec95a Compare February 25, 2025 15:30
Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 4 files at r2, 7 of 8 files at r3, 5 of 5 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion


installer-downloader/src/winapi_impl/mod.rs line 36 at r4 (raw file):

        EnvError::Arch => "Failed to detect CPU architecture",
    };
    nwg::fatal_message(title, content)

We could either use modal_fatal_message or display this error before creating the AppWindow at all. Currently, the MessageBox dialog doesn't prevent the main window from being focused.

@dlon dlon force-pushed the installer-downloader branch 2 times, most recently from db28ddb to 165d53d Compare February 27, 2025 08:21
@MarkusPettersson98 MarkusPettersson98 force-pushed the detect-native-arch branch 3 times, most recently from 9d51950 to bca0791 Compare February 27, 2025 10:04
Copy link
Contributor Author

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 10 of 15 files reviewed, all discussions resolved (waiting on @dlon)


installer-downloader/src/winapi_impl/mod.rs line 36 at r4 (raw file):

Previously, dlon (David Lönnhager) wrote…

We could either use modal_fatal_message or display this error before creating the AppWindow at all. Currently, the MessageBox dialog doesn't prevent the main window from being focused.

Done

Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 4 of 4 files at r5, 3 of 3 files at r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@MarkusPettersson98 MarkusPettersson98 merged commit c9a6b57 into installer-downloader Feb 27, 2025
44 of 51 checks passed
@MarkusPettersson98 MarkusPettersson98 deleted the detect-native-arch branch February 27, 2025 10:56
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.

2 participants