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

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

Merged
merged 3 commits into from
Jun 11, 2024
Merged

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

merged 3 commits into from
Jun 11, 2024

Conversation

bgermann
Copy link
Contributor

@bgermann bgermann commented Jun 7, 2024

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

    CLA assistant check
    All committers have signed the CLA.

    Copy link
    Contributor

    qodo-merge-pro 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

    Copy link
    Contributor

    qodo-merge-pro 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

    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.

    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

    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

    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
    20 checks passed
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Oct 29, 2024
    Co-authored-by: Boni García <boni.garcia@uc3m.es>
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    4 participants