Replace host_windows condition with @platforms//os:windows#3909
Replace host_windows condition with @platforms//os:windows#3909davido wants to merge 1 commit intobazel-contrib:5.xfrom
Conversation
Bazel 8 emits warnings for deprecated bazel_tools condition targets such as @bazel_tools//src/conditions:host_windows_x64_constraint. rules_nodejs still uses @bazel_tools//src/conditions:host_windows in a few widely used rules (e.g. js_library, npm_link, pkg_npm), causing consumers to see large numbers of warnings from generated npm targets. Replace these host Windows checks with @platforms//os:windows in the affected user-facing rules. This is a minimal change to eliminate the Bazel 8 warning flood without attempting a full migration of all bazel_tools condition usages. Architecture-specific and toolchain-selection cases are intentionally left unchanged, as they require a more careful migration. Fixes Bazel 8 warnings reported in bazel-contrib#2440
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a66fc4faa3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| srcs = srcs, | ||
| is_windows = select({ | ||
| "@bazel_tools//src/conditions:host_windows": True, | ||
| "@platforms//os:windows": True, |
There was a problem hiding this comment.
Keep Windows copy mode tied to host, not target platform
This select now uses @platforms//os:windows, which follows the target platform, but _copy_to_bin_impl uses is_windows to choose copy_cmd vs copy_bash for the action runner; copy_cmd invokes cmd.exe (third_party/github.com/bazelbuild/bazel-skylib/rules/private/copy_file_private.bzl, copy_cmd). On a Linux/macOS host building Windows targets (for example with --platforms=@platforms//os:windows), this flips to cmd.exe even though execution is not on Windows, causing copy actions to fail. The previous host_windows condition avoided that by matching host OS.
Useful? React with 👍 / 👎.
|
The CI failure appears to come from the Bazel 5.0.0 test lane: This change is specifically targeting Bazel 8, where legacy @bazel_tools//src/conditions:host_* targets are deprecated and produce a large number of analysis warnings. Using @platforms//os:windows is the recommended replacement in newer Bazel versions and resolves the warning flood for users on Bazel 8. Given that, this PR intentionally focuses on improving behavior with current Bazel versions rather than maintaining compatibility with discontinued Bazel 5 setups. Please let me know if Bazel 5 compatibility is still a requirement for this repository; otherwise this change should be correct for newer Bazel releases. |
Bazel 8 emits warnings for deprecated bazel_tools condition targets such as @bazel_tools//src/conditions:host_windows_x64_constraint.
rules_nodejs still uses @bazel_tools//src/conditions:host_windows in a few widely used rules (e.g. js_library, npm_link, pkg_npm), causing consumers to see large numbers of warnings from generated npm targets.
Replace these host Windows checks with @platforms//os:windows in the affected user-facing rules. This is a minimal change to eliminate the Bazel 8 warning flood without attempting a full migration of all bazel_tools condition usages.
Architecture-specific and toolchain-selection cases are intentionally left unchanged, as they require a more careful migration.
Fixes Bazel 8 warnings reported in #2440
PR Checklist
Please check if your PR fulfills the following requirements:
No tests/docs needed: this change only replaces deprecated condition labels without changing behavior.
PR Type
What kind of change does this PR introduce?
What is the current behavior?
With Bazel 8, builds using rules_nodejs produce a large number of warnings due to usage of deprecated condition targets such as:
This happens because several widely used rules (
js_library,npm_link,pkg_npm, etc.) still rely on legacy@bazel_tools//src/conditions:host_windows.As a result, consumers see a flood of warnings during analysis, especially from generated
npmtargets.Issue: #2440
What is the new behavior?
The affected host Windows checks are replaced with
@platforms//os:windowsin user-facing rules.This removes the Bazel 8 deprecation warnings while preserving existing behavior.
The change is intentionally minimal and does not migrate all remaining
bazel_tools//src/conditionsusages (e.g. architecture-specific or toolchain-related cases).Does this PR introduce a breaking change?
Other information
Although the 5.x line is no longer actively developed, this small fix significantly improves the user experience for consumers migrating to Bazel 8 by eliminating a large volume of warnings.
Having this included in a final 5.8.x patch release would make migration smoother until users can fully move to newer alternatives (e.g.
rules_js).