Skip to content

[rust] Fix local architecture discovery in Selenium Manager#11611

Merged
bonigarcia merged 2 commits intotrunkfrom
se_mgr_arch
Mar 6, 2023
Merged

[rust] Fix local architecture discovery in Selenium Manager#11611
bonigarcia merged 2 commits intotrunkfrom
se_mgr_arch

Conversation

@bonigarcia
Copy link
Copy Markdown
Member

Description

This PR fixes a bug reported in #11517, related to the architecture discovery (e.g., arm64, x86_64) in Selenium Manager.

Motivation and Context

So far, the Selenium Manager logic relied on a Rust standard constant called std::env::consts::ARCH for the architecture discovery. According to its doc, this constant is "A string describing the architecture of the CPU that is currently in use". Nevertheless, when compiled for a x86_64, like in CI (for instance, for macOS, using cargo build --release --target x86_64-apple-darwin), that constant is always x86_64 (even when executed in a macOS M1). The solution I found to fix this issue is to rely on the command uname (in UNIX/Linux) and the environment variable (PROCESSOR_ARCHITECTURE) in Windows for the automatic discovery of the underlying architecture.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@bonigarcia bonigarcia force-pushed the se_mgr_arch branch 2 times, most recently from 553f803 to 849b728 Compare February 2, 2023 14:43
@bonigarcia bonigarcia added the C-rust Rust code is mostly Selenium Manager label Feb 20, 2023
@bonigarcia bonigarcia requested a review from diemol February 20, 2023 14:24
@diemol
Copy link
Copy Markdown
Member

diemol commented Mar 3, 2023

I see issue #11517 is closed, is this still needed?

@bonigarcia
Copy link
Copy Markdown
Member Author

Yes, this PR still needs to be merged.

@diemol
Copy link
Copy Markdown
Member

diemol commented Mar 6, 2023

👍
Can you please fix the conflicts so we can merge?

@bonigarcia bonigarcia merged commit d7b0b09 into trunk Mar 6, 2023
@bonigarcia bonigarcia deleted the se_mgr_arch branch March 6, 2023 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-rust Rust code is mostly Selenium Manager

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants