PR Code Suggestions ✨Explore these optional code suggestions: | Category | Suggestion | 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 theselect statement,
useselects.config_setting_group to create a mutually exclusive condition. This
ensures that building from source only occurs when the OS matches and neither
theuse_pinned_browser norstamp 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 theselect logic that would cause build failures when multiple independent conditions are met, and it provides the correct, idiomatic Bazel fix usingconfig_setting_group to ensure mutual exclusivity. | High | Learned
best practice | Centralize repeated select logic
Factor the repeatedselect() 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", )Suggestion importance[1-10]: 6__ Why:
Relevant best practice - Replace ad-hoc duplication with shared helpers/utilities or macros to centralize repeated logic. | Low | | |
|
diemol left a comment
Uh oh!
There was an error while loading.Please reload this page.
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
@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 --> EFile Walkthrough
BUILD.bazel
Add conditional build vs download logic for Selenium Managercommon/manager/BUILD.bazel
select()statements
use_pinned_browserorstampflags are set//rust:selenium-manager-*targetsotherwise
aliases