Skip to content

Conversation

@titusfortner
Copy link
Member

@titusfortner titusfortner commented Dec 15, 2025

Note: This code includes #16743 for it to work

User description

💥 What does this PR do?

This code fixed constant build churn by always downloading latest manager: #13314
But this doesn't work when you change selenium manager code locally.

Conditional is now

  • if pin_browsers - download instead of build
  • if stamp - download instead of build
  • else build instead of download

@bonigarcia / @diemol does this look right to you?


PR Type

Enhancement


Description

  • Conditionally build or download Selenium Manager based on build flags

  • Download when using pinned browsers or stamped builds

  • Build locally when developing Selenium Manager code

  • Reduces build churn while supporting local development


Diagram Walkthrough

flowchart LR A["Build Configuration"] --> B{Check Conditions} B -->|use_pinned_browser| C["Download Manager"] B -->|stamp| C B -->|else| D["Build Manager Locally"] C --> E["Selenium Manager Binary"] D --> E 
Loading

File Walkthrough

Relevant files
Build
BUILD.bazel
Add conditional build vs download logic for Selenium Manager

common/manager/BUILD.bazel

  • Replaced static download references with conditional select()
    statements
  • Added logic to download when use_pinned_browser or stamp flags are set
  • Added logic to build locally from //rust:selenium-manager-* targets
    otherwise
  • Applied same conditional pattern to Linux, macOS, and Windows manager
    aliases
+18/-3   

@selenium-ci selenium-ci added the B-build Includes scripting, bazel and CI integrations label Dec 15, 2025
@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Dec 15, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label
@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Dec 15, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Prevent build failures from conflicting conditions
Suggestion Impact:The commit addressed the conflicting select-condition problem, but by simplifying the select() mappings: it removed the separate "//common:stamp" and OS-specific keys and switched the default branch to build from source (//rust:selenium-manager-*) while keeping the use_pinned_browser override. This avoids the original overlapping-condition failure without introducing config_setting_group.

code diff:

@@ -13,9 +13,7 @@ name = "selenium-manager-linux", actual = select({ "//common:use_pinned_browser": "@download_sm_linux//file", - "//common:stamp": "@download_sm_linux//file", - "//common:linux": "//rust:selenium-manager-linux", - "//conditions:default": "@download_sm_linux//file", + "//conditions:default": "//rust:selenium-manager-linux", }), ) @@ -23,9 +21,7 @@ name = "selenium-manager-macos", actual = select({ "//common:use_pinned_browser": "@download_sm_macos//file", - "//common:stamp": "@download_sm_macos//file", - "//common:macos": "//rust:selenium-manager-macos", - "//conditions:default": "@download_sm_macos//file", + "//conditions:default": "//rust:selenium-manager-macos", }), ) @@ -33,8 +29,6 @@ name = "selenium-manager-windows", actual = select({ "//common:use_pinned_browser": "@download_sm_windows//file", - "//common:stamp": "@download_sm_windows//file", - "//common:windows": "//rust:selenium-manager-windows", - "//conditions:default": "@download_sm_windows//file", + "//conditions:default": "//rust:selenium-manager-windows", }),

To prevent build failures from conflicting conditions in the select statement,
use selects.config_setting_group to create a mutually exclusive condition. This
ensures that building from source only occurs when the OS matches and neither
the use_pinned_browser nor stamp flags are set.

common/manager/BUILD.bazel [12-20]

+load("//bazel:rules.bzl", "selects") + +selects.config_setting_group( + name = "build_sm_linux_from_source", + match_all = [ + "//common:linux", + ], + match_none = [ + "//common:use_pinned_browser", + "//common:stamp", + ], +) + alias( name = "selenium-manager-linux", actual = select({ "//common:use_pinned_browser": "@download_sm_linux//file", "//common:stamp": "@download_sm_linux//file", - "//common:linux": "//rust:selenium-manager-linux", + ":build_sm_linux_from_source": "//rust:selenium-manager-linux", "//conditions:default": "@download_sm_linux//file", }), )

[Suggestion processed]

Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a critical flaw in the select logic that would cause build failures when multiple independent conditions are met, and it provides the correct, idiomatic Bazel fix using config_setting_group to ensure mutual exclusivity.

High
Learned
best practice
Centralize repeated select logic

Factor the repeated select() mapping into a small Starlark macro (in a .bzl
file) so the conditional logic lives in one place and OS-specific values are
passed as parameters. This reduces duplication and prevents future changes from
being applied inconsistently across platforms.

common/manager/BUILD.bazel [12-40]

-alias( +load("//common/manager:selenium_manager_alias.bzl", "selenium_manager_alias") + +selenium_manager_alias( name = "selenium-manager-linux", - actual = select({ - "//common:use_pinned_browser": "@download_sm_linux//file", - "//common:stamp": "@download_sm_linux//file", - "//common:linux": "//rust:selenium-manager-linux", - "//conditions:default": "@download_sm_linux//file", - }), + download = "@download_sm_linux//file", + local = "//rust:selenium-manager-linux", + os_condition = "//common:linux", ) -alias( +selenium_manager_alias( name = "selenium-manager-macos", - actual = select({ - "//common:use_pinned_browser": "@download_sm_macos//file", - "//common:stamp": "@download_sm_macos//file", - "//common:macos": "//rust:selenium-manager-macos", - "//conditions:default": "@download_sm_macos//file", - }), + download = "@download_sm_macos//file", + local = "//rust:selenium-manager-macos", + os_condition = "//common:macos", ) -alias( +selenium_manager_alias( name = "selenium-manager-windows", - actual = select({ - "//common:use_pinned_browser": "@download_sm_windows//file", - "//common:stamp": "@download_sm_windows//file", - "//common:windows": "//rust:selenium-manager-windows", - "//conditions:default": "@download_sm_windows//file", - }), + download = "@download_sm_windows//file", + local = "//rust:selenium-manager-windows", + os_condition = "//common:windows", )
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Replace ad-hoc duplication with shared helpers/utilities or macros to centralize repeated logic.

Low
  • Update
Copy link
Member

@diemol diemol left a comment

Choose a reason for hiding this comment

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

I think this makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-build Includes scripting, bazel and CI integrations Review effort 2/5

4 participants