Replace host_windows condition with @platforms//os:windows#3909
Replace host_windows condition with @platforms//os:windows#3909davido wants to merge 2 commits 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
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
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".
internal/common/copy_to_bin.bzl
Outdated
| srcs = srcs, | ||
| is_windows = select({ | ||
| "@bazel_tools//src/conditions:host_windows": True, | ||
| "@platforms//os:windows": True, |
There was a problem hiding this comment.
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. |
Switch from the upstream rules_nodejs 5.8.5 release tarball to a forked commit that includes the Bazel 8 compatibility fix for deprecated host_* conditions. With Bazel 8, legacy targets such as @bazel_tools//src/conditions:host_windows_x64_constraint produce a large number of analysis warnings. rules_nodejs 5.8.5 still uses @bazel_tools//src/conditions:host_windows in several widely used rules, causing a warning flood from generated npm targets. The fork replaces these host-based conditions with @platforms//os:windows, eliminating the warnings for Bazel 8 users while preserving existing behavior. Pin the dependency to a specific fork commit for reproducibility and avoid adding a local patch to the Gerrit source tree. This is a temporary workaround until the fix is available in an upstream rules_nodejs release. See [1]. [1] bazel-contrib/rules_nodejs#3909 Release-Notes: skip Change-Id: I96f437d009ad59ecae73679304cd00dfd44ce6df
Gerrit does not support Windows builds, so stop selecting Windows-specific behavior in the forked rules_nodejs snapshot. Hardcode the non-Windows path in js_library and related helper rules, and always use the .sh packaging entry points. This avoids Bazel 8 deprecation warnings from host_windows condition targets and also avoids the visibility regression triggered by replacing them with @platforms//os:windows. This keeps Linux and macOS builds working while making the fork explicitly Unix-only.
davido
force-pushed
the
fix/bazel8-host-conditions
branch
from
888d216 to
e4fed3c
Compare
1 participant
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).