Skip to content

Deprecate ctx.configuration.host_path_separator #25920

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Apr 22, 2025

Using this field for its intended purpose is essentially almost incorrect as it is based on the OS of the machine running Bazel, not any execution platform. It can be used to sneakily reason about the local machine in a build rule, but that's discouraged and should be done in a repository rule instead.

@fmeum fmeum requested review from meteorcloudy and Copilot April 22, 2025 09:53
@github-actions github-actions bot added the awaiting-review PR is awaiting review from an assigned reviewer label Apr 22, 2025
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR deprecates the use of ctx.configuration.host_path_separator by updating its documentation to discourage its usage and suggest alternative approaches.

  • Updates the documentation for host_path_separator to highlight deprecation and provide alternative guidance.
  • Clarifies the inherent limitations of using the OS-dependent separator in Bazel build rules.
Comments suppressed due to low confidence (1)

src/main/java/com/google/devtools/build/lib/starlarkbuildapi/BuildConfigurationApi.java:55

  • Consider adding the @deprecated annotation to the getHostPathSeparator() method to signal deprecation at the code level.
String getHostPathSeparator();

Using this field for its intended purpose is essentially almost incorrect as it is based on the OS of the machine running Bazel, not any execution platform. It can be used to sneakily reason about the local machine in a build rule, but that's discouraged and should be done in a repository rule instead.
@fmeum fmeum force-pushed the deprecate-host-separator branch from 35db2a7 to b115958 Compare April 22, 2025 09:59
@meteorcloudy meteorcloudy requested a review from katre April 22, 2025 13:43
@@ -43,7 +43,16 @@ public interface BuildConfigurationApi extends StarlarkValue {
@StarlarkMethod(
name = "host_path_separator",
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to offer an easy drop-in replacement for this API based on the execution platform?

It's been used quite widely:
https://sourcegraph.com/search?q=context:global+%22host_path_separator%22+file:.*.bzl&patternType=keyword&sm=0

Copy link
Member

Choose a reason for hiding this comment

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

I had a discussion a while back (possibly with @susinmotion ?) about adding a ctx.exec_platform_has_constraint method (especially since this is currently available to native rules but not Starlark).

Would that make this easier to migrate?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I do believe those are used mostly in rule implementations to construct build actions. Just wonder if we can add a helper function somewhere (bazel_skylib) to be a drop-in replacement?

Copy link
Member

Choose a reason for hiding this comment

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

Not without support for finding out whether the exec platform uses windows-style separators (which is, basically, ctx.exec_platform_has_constraint).

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see, I confused it with ctx.target_platform_has_constraint and thought it's already available. Then, yes, I do think adding ctx.exec_platform_has_constraint is probably desired. @fmeum WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ctx.exec_platform_has_constraint doesn't quite cut it since there could be multiple execution platforms in a rule. We could do ctx.exec_groups["my_group"].exec_platform_has_constraint(), but then we would need a convenient way to name the default exec group.

@iancha1992 iancha1992 added the team-Configurability platforms, toolchains, cquery, select(), config transitions label Apr 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review PR is awaiting review from an assigned reviewer team-Configurability platforms, toolchains, cquery, select(), config transitions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants