SeleniumHQ / selenium

A browser automation framework and ecosystem.
https://selenium.dev
Apache License 2.0
29.77k stars 8.02k forks source link

[rust] Enable Firefox nightly for linux on arm64 #14099

Closed bgermann closed 3 weeks ago

bgermann commented 3 weeks ago

Description

Mozilla provides nightly Firefox releases for ARM64 Linux now. Make selenium-manager aware of them.

Motivation and Context

On ARM64 Linux, selenium-manager downloads binaries for x86_64 even though there are releases for ARM64.

Types of changes

Checklist


PR Type

Enhancement


Description


Changes walkthrough ๐Ÿ“

Relevant files
Enhancement
firefox.rs
Add ARM64 support for nightly Firefox on Linux                     

rust/src/firefox.rs
  • Added support for ARM64 architecture in nightly Firefox builds.
  • Modified platform label assignment logic for nightly builds.
  • +5/-1     

    ๐Ÿ’ก PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    CLAassistant commented 3 weeks ago

    CLA assistant check
    All committers have signed the CLA.

    codiumai-pr-agent-pro[bot] commented 3 weeks ago

    PR Review ๐Ÿ”

    โฑ๏ธ Estimated effort to review [1-5] 2, because the changes are limited to a specific functionality within a single file and involve a straightforward condition addition. The logic is simple and the impact is localized.
    ๐Ÿงช Relevant tests No
    โšก Possible issues Possible Bug: The code does not handle cases other than ARM64 and default x86_64 for nightly builds. If there are other architectures supported in the future, they will default to 'linux-x86_64', which might not be correct.
    ๐Ÿ”’ Security concerns No
    codiumai-pr-agent-pro[bot] commented 3 weeks ago

    PR Code Suggestions โœจ

    CategorySuggestion                                                                                                                                    Score
    Maintainability
    Use a match statement to improve readability and maintainability when setting the platform_label ___ **Consider using a match statement for better readability and maintainability when checking
    the architecture and setting the platform_label.** [rust/src/firefox.rs [569-579]](https://github.com/SeleniumHQ/selenium/pull/14099/files#diff-a223ca8815bf06c61bd96d36c5ebae1789d76dbc4d91ae6aa82d47f6d087f611R569-R579) ```diff -if X32.is(arch) { - platform_label = "linux-i686"; -} else if self.is_nightly(browser_version) { - if ARM64.is(arch) { - platform_label = "linux64-aarch64"; - } else { - platform_label = "linux64"; - } -} else { - platform_label = "linux-x86_64"; -} +platform_label = match (X32.is(arch), self.is_nightly(browser_version), ARM64.is(arch)) { + (true, _, _) => "linux-i686", + (false, true, true) => "linux64-aarch64", + (false, true, false) => "linux64", + (false, false, _) => "linux-x86_64", +}; ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 7 Why: The suggestion to replace nested if-else with a match statement is valid and improves readability and maintainability of the code. However, it is not addressing a major bug or security issue, hence the score is 7.
    7
    Enhancement
    Add a log statement to provide more visibility into the platform label being set ___ **Add a log statement to provide more visibility into which platform label is being set,
    which can help in debugging.** [rust/src/firefox.rs [569-579]](https://github.com/SeleniumHQ/selenium/pull/14099/files#diff-a223ca8815bf06c61bd96d36c5ebae1789d76dbc4d91ae6aa82d47f6d087f611R569-R579) ```diff if X32.is(arch) { platform_label = "linux-i686"; } else if self.is_nightly(browser_version) { if ARM64.is(arch) { platform_label = "linux64-aarch64"; } else { platform_label = "linux64"; } } else { platform_label = "linux-x86_64"; } +log::info!("Platform label set to: {}", platform_label); ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 5 Why: Adding a log statement is a good practice for debugging, but it's a minor enhancement and not critical to the functionality of the code.
    5
    titusfortner commented 3 weeks ago

    Looks like this is related to #13793

    1. How does Selenium manager run on arm64 when it is only compiled for x86?
    2. We need to also be able to get the arm64 driver for it to matter that we are getting the arm64 browser
    bgermann commented 3 weeks ago

    Looks like this is related to #13793

    1. How does Selenium manager run on arm64 when it is only compiled for x86?

    I compile it myself on for arm64. Generally it runs.

    1. We need to also be able to get the arm64 driver for it to matter that we are getting the arm64 browser

    That is already the case.

    bgermann commented 3 weeks ago

    I do not think that the failing test (Windows-only: fatal error LNK1181: cannot open input file 'windows.0.52.0.lib') is related to this change as this codepath is not relevant on Windows.

    bonigarcia commented 3 weeks ago

    No, that error is not related. In theory, that should be fixed in the trunk branch.