Skip to content

Conversation

bonigarcia
Copy link
Member

@bonigarcia bonigarcia commented Oct 1, 2025

User description

🔗 Related Issues

💥 What does this PR do?

This PR set the Rust compiler version to 1.89.0 in WORKSPACE. This will allow us to use the latest features of the Rust language. This PR also contains some smell-fixes only possible with this version.

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

  • Update

PR Type

Enhancement


Description

  • Update Rust compiler version to 1.89.0 in WORKSPACE

  • Refactor conditional logic using modern Rust syntax

  • Replace deprecated method calls with newer alternatives

  • Switch linker from gold to lld in build configuration


Diagram Walkthrough

flowchart LR
  A["WORKSPACE"] -- "update version" --> B["Rust 1.89.0"]
  B -- "enables" --> C["Modern Syntax"]
  C -- "refactor" --> D["Conditional Logic"]
  C -- "update" --> E["Method Calls"]
  F["Build Config"] -- "switch linker" --> G["lld"]
Loading

File Walkthrough

Relevant files
Configuration changes
WORKSPACE
Set Rust version to 1.89.0                                                             

WORKSPACE

  • Update Rust toolchain version from default to 1.89.0
+1/-1     
BUILD
Update linker configuration                                                           

common/remote-build/cc/BUILD

  • Switch linker from gold to lld in link flags
+1/-1     
Enhancement
downloads.rs
Update iterator method call                                                           

rust/src/downloads.rs

  • Replace deprecated last() with next_back() method
+1/-1     
lib.rs
Modernize conditional logic with let-chains                           

rust/src/lib.rs

  • Refactor nested if-let statements using let-chains syntax
  • Simplify conditional logic for snap path handling
+10/-11 
lock.rs
Refactor lock handling with let-chains                                     

rust/src/lock.rs

  • Replace nested if statements with let-chains pattern
  • Simplify lock path existence checking logic
+4/-6     
main.rs
Modernize error handling syntax                                                   

rust/src/main.rs

  • Convert nested if-let to let-chains syntax
  • Replace ternary-like expression with if-else block
  • Improve error handling readability
+23/-23 

@selenium-ci selenium-ci added C-rust Rust code is mostly Selenium Manager B-manager Selenium Manager labels Oct 1, 2025
Copy link
Contributor

qodo-merge-pro bot commented Oct 1, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Panic on unwrap

Description: Calling selenium_manager.find_best_driver_from_cache().unwrap() inside error handling can
panic on None/Err, potentially turning a recoverable failure into a crash and enabling
denial-of-service via triggering this path.
main.rs [258-276]

Referred Code
    && let Some(best_driver_from_cache) =
        selenium_manager.find_best_driver_from_cache().unwrap()
{
    log.debug_or_warn(
        format!(
            "There was an error managing {} ({}); using driver found in the cache",
            selenium_manager.get_driver_name(),
            err
        ),
        selenium_manager.is_offline(),
    );
    log_driver_and_browser_path(
        log,
        &best_driver_from_cache,
        &selenium_manager.get_browser_path_or_latest_from_cache(),
        selenium_manager.get_receiver(),
    );
    flush_and_exit(OK, log, Some(err));
}
Unsafe file deletion

Description: Deleting a filesystem path from get_lock_path() without validating that it resides in an
expected directory could allow unintended file deletion if the path is manipulated
elsewhere.
lock.rs [73-77]

Referred Code
if let Some(lock) = get_lock_path()
    && lock.exists()
{
    fs::remove_file(lock).unwrap_or_default();
}
Ticket Compliance
🟡
🎫 #5678
🔴 Investigate and resolve "Error: ConnectFailure (Connection refused)" occurring after the
first ChromeDriver instantiation on Ubuntu 16.04 with Chrome 65 and ChromeDriver 2.35
(Selenium 3.9.0).
Provide a reliable fix or mitigation so subsequent ChromeDriver instances do not log the
connection failure.
Ideally include steps or code changes that address repeated instantiation behavior.
🟡
🎫 #1234
🔴 Ensure that clicking links with JavaScript in the href triggers as in Selenium 2.47.1,
addressing regression seen in 2.48.x with Firefox 42.
Provide compatibility or fix in click handling so JS href executes.
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
No custom compliance provided

Follow the guide to enable custom compliance check.

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Copy link
Contributor

qodo-merge-pro bot commented Oct 1, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Avoid panicking in error handler

Replace .unwrap() with let Ok(Some(...)) pattern matching to safely handle the
Result from find_best_driver_from_cache() and prevent a potential panic in the
error handler.

rust/src/main.rs [257-276]

 if selenium_manager.is_fallback_driver_from_cache()
-    && let Some(best_driver_from_cache) =
-        selenium_manager.find_best_driver_from_cache().unwrap()
+    && let Ok(Some(best_driver_from_cache)) = selenium_manager.find_best_driver_from_cache()
 {
     log.debug_or_warn(
         format!(
             "There was an error managing {} ({}); using driver found in the cache",
             selenium_manager.get_driver_name(),
             err
         ),
         selenium_manager.is_offline(),
     );
     log_driver_and_browser_path(
         log,
         &best_driver_from_cache,
         &selenium_manager.get_browser_path_or_latest_from_cache(),
         selenium_manager.get_receiver(),
     );
     flush_and_exit(OK, log, Some(err));
 }
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a potential panic caused by .unwrap() within an error handling block, which would crash the program instead of handling the error gracefully.

Medium
Learned
best practice
Robustly extract filename segment

Avoid taking a mutable segments for a single next_back() call and guard for
trailing slashes explicitly. Use rsplit('/') on the path string to robustly
extract the last non-empty segment.

rust/src/downloads.rs [55-58]

-.path_segments()
-.and_then(|mut segments| segments.next_back())
-.and_then(|name| if name.is_empty() { None } else { Some(name) })
-.unwrap_or("tmp.bin");
+let target_name = response
+    .url()
+    .path()
+    .rsplit('/')
+    .find(|s| !s.is_empty())
+    .unwrap_or("tmp.bin");
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Validate inputs and states early to avoid logic errors; ensure iterator usage matches expected type and handles empty paths robustly.

Low
  • Update

@bonigarcia bonigarcia force-pushed the sm_fix_rust_version branch from bed718c to 78e5c61 Compare October 1, 2025 17:38
@bonigarcia bonigarcia merged commit 8e84f0d into trunk Oct 1, 2025
28 checks passed
@bonigarcia bonigarcia deleted the sm_fix_rust_version branch October 1, 2025 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-manager Selenium Manager C-rust Rust code is mostly Selenium Manager Review effort 2/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants