Skip to content

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

Merged
titusfortner merged 3 commits intoSeleniumHQ:trunkfrom
bgermann:linux-aarch64
Jun 11, 2024
Merged

[rust] Enable Firefox nightly for linux on arm64#14099
titusfortner merged 3 commits intoSeleniumHQ:trunkfrom
bgermann:linux-aarch64

Conversation

@bgermann
Copy link
Contributor

@bgermann bgermann commented Jun 7, 2024 •
edited
Loading

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

  • 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.

PR Type

Enhancement

Description

  • Added support for downloading ARM64 binaries for nightly Firefox builds on Linux.
  • Modified the platform label assignment to distinguish between ARM64 and other architectures for nightly builds.

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
    Copy link

    CLAassistant commented Jun 7, 2024 •
    edited
    Loading

    CLA assistant check
    All committers have signed the CLA.

    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented Jun 7, 2024

    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

    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented Jun 7, 2024

    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]

    -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]

     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 titusfortner requested a review from bonigarcia June 7, 2024 16:26
    @titusfortner
    Copy link
    Member

    titusfortner commented Jun 7, 2024

    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
    Copy link
    Contributor Author

    bgermann commented Jun 7, 2024

    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.

    bonigarcia
    bonigarcia approved these changes Jun 7, 2024
    View reviewed changes
    Copy link
    Member

    @bonigarcia bonigarcia left a comment

    Choose a reason for hiding this comment

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

    LGTM

    @bgermann
    Copy link
    Contributor Author

    bgermann commented Jun 11, 2024

    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
    Copy link
    Member

    bonigarcia commented Jun 11, 2024

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

    @titusfortner titusfortner merged commit 7af4f19 into SeleniumHQ:trunk Jun 11, 2024
    @seedysoft seedysoft mentioned this pull request Jun 21, 2024
    Merged
    @MiguelPelegrina MiguelPelegrina mentioned this pull request Jul 12, 2024
    Merged
    @aravindb26 aravindb26 mentioned this pull request Jul 16, 2024
    Open
    @x-way x-way mentioned this pull request Jul 24, 2024
    Closed
    @x-way x-way mentioned this pull request Aug 10, 2024
    Closed
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Oct 29, 2024
    @bgermann @bonigarcia @sandeepsuryaprasad
    [rust] Enable Firefox nightly for linux on arm64 (SeleniumHQ#14099)
    c5764e9 
    Co-authored-by: Boni García <boni.garcia@uc3m.es>
    @bonigarcia bonigarcia mentioned this pull request Dec 12, 2024
    Closed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @bonigarcia bonigarcia bonigarcia approved these changes

    Assignees

    No one assigned

    Labels

    P-enhancement PR with a new feature Review effort [1-5]: 2

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    4 participants

    @bgermann @CLAassistant @titusfortner @bonigarcia