Skip to content
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

Add dirname variants of predefined source/output path variables. #23518

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

Conversation

jmillikin
Copy link
Contributor

These are similar to existing $(execpath) and friends, but return the path of the artifact's directory rather than of the artifact itself.

Fixes #23516

@jmillikin jmillikin requested a review from a team as a code owner September 5, 2024 05:20
@jmillikin jmillikin requested review from gregestren and removed request for a team September 5, 2024 05:20
@github-actions github-actions bot added team-Configurability platforms, toolchains, cquery, select(), config transitions awaiting-review PR is awaiting review from an assigned reviewer labels Sep 5, 2024
These are similar to existing `$(execpath)` and friends, but return the
path of the artifact's directory rather than of the artifact itself.
Copy link
Contributor

@tjgq tjgq left a comment

Choose a reason for hiding this comment

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

cc @comius

Would it be more composable to provide a $(dirname ...) primitive, i.e., a user would instead write $(dirname $(execpath ...)) or $(dirname $(rootpath ...))?

@fmeum
Copy link
Collaborator

fmeum commented Sep 5, 2024

@illicitonion Didn't you file an issue for this a while ago? Can't find it right now, but it also had relevant discussion.

@fmeum
Copy link
Collaborator

fmeum commented Sep 5, 2024

Nvm, found it: #23139

@keith
Copy link
Member

keith commented Oct 18, 2024

any idea who we should ping to review this one?

@fmeum
Copy link
Collaborator

fmeum commented Oct 19, 2024

Reviewing this change is probably quite easy compared to validating its design choice (@tjgq suggested an alternative above, #23139 has another one).

I would personally benefit from a short write-up that lists the use cases and how each of the proposed APIs would address them (or not).

@jmillikin
Copy link
Contributor Author

jmillikin commented Oct 19, 2024

Proposal A: $(execpath_dirname //some:label)

  • Returns the directory path of a single-file label, equivalent to File.dirname.
  • As proposed, it doesn't work for multi-file labels.
    • There would be no semantics problem if restricted to cases where all files identified by the label are in the same directory, but that also limits its utility.
    • Unclear semantics if allowed for labels that have multiple files in different directories, what path should be returned for a target filegroup(srcs = ["a/b", "a/c", "a/d/e"]) ?

Proposal A-2: $(execpath_dirname //some:multi-file-label)

  • Returns the unique set of directory paths for all files in the label, similar to the dirname shell command.
  • Unclear what the user would do with such an output, since usually there needs to be some interpolation (like adding -isystem between paths to include files' directories)
  • Could have an API somewhat like Args: $(execpath_dirname "-iquote" //some:label) might produce "-iquote a/b -iquote c/d" but that's blurring the line pretty badly between make variables (conceptually simple) and Starlark (... generally not considered quite as simple).

Proposal B: $(execpath //some/package:__pkg__)

  • Returns the path to a package, equivalent to $(execpath_dirname //some/package:BUILD).
  • The provided example is obtaining the path to a third-party C library's include/ directory for computing copts=, but $(execpath ...) is probably not the right approach for that anyway because it ignores _virtual_includes and would break for labels containing both source files and generated files.
  • Doesn't work with aliases.
  • Generally with trying to inspect paths for Files the thing the user wants is related to the File, not its generating package.

Proposal C: $(dirname $(execpath //some:label))

  • Instead of $(execpath_dirname ...) and $(rootpath_dirname ...), the $(dirname ...) function generically allows the directory name to be extracted from File paths.
  • Implementation would require either reparsing the path (bad), or changing $(execpath) and friends to return Path instead of String (and plumb that through various layers).
  • If $(execpath //:foo) returns foo.txt, what does $(dirname $(dirname $(execpath //:foo))) return? Is the answer the same on different platforms?
    • If it returns "." then $(dirname ...) is implicitly scoped to the build root. Is it allowed to generate paths that are "in between" the build root and the artifacts, such as "bazel-bin/" from being called on a generated file more times than its path has components?
    • If it returns ".." then $(dirname ...) is allowed to generate arbitrary paths outside the build root, which would be surprising behavior for a make-variable builtin function.

Proposal D: $(execpath-1 //some:label)

  • Allows more concise notation for nested $(dirname ...), for example $(dirname $(dirname $(dirname //some:label))) could be written $(execpath-3 //some:label).
  • Same benefits (more combinator-like API) and drawbacks (unclear semantics if the result would be a valid filesystem path but not a valid Bazel path).
  • The word dirname doesn't make an appearance, and the -N notation for stripping path components isn't widespread enough for users to guess its purpose without a deep inspection of the make variable docs.

Proposal E: $(user_defined_execpath_dirname //some:label)

  • Similar to how make variables can be injected into a target via the toolchains= parameter, allow Starlark functions to be injected.
  • A user who has complex make-variable expansion needs could write execpath_dirname.bzl that has execpath_dirname = make_variable_function(_execpath_dirname, args = [attr.label()]) and returns it via a TemplateVariableInfo.
  • Fully self-service for authors of build rules (and rulesets). rules_cc could provide things like $(cc_iquote_include_dirs //some:label).
  • Much more API surface, more difficult to implement (requires lots of plumbing).
  • It would be nice if common use cases such as "dirname of a single-file label" could be satisfied without the user having to learn a new corner of the extension API.

@jmillikin
Copy link
Contributor Author

Gentle ping @fmeum Was the above list of use cases and proposals what you were looking for, or would you prefer a more formal design doc -style writeup?

@fmeum
Copy link
Collaborator

fmeum commented Nov 28, 2024

This is what I (personally) have been looking for. It's very rich in information, thanks!

Regarding $(dirname ...): Since paths obtained via location expansion are always slash-separated, I wonder whether we could restrict this to operating on slash-separated paths as well. If we then cap it off at . for the execroot (failing if used again), I could see this being a pretty versatile approach. It would still be limited to single-file labels.

My favorites are A and C, but I don't know which of them I'd prefer.

@armandomontanez
Copy link

@matts1 added ways to represent directories to bazel-skylib, it'd be really nice if we could ensure whatever path we take plays well with that.

@mentalnote
Copy link

I've worked around a need for this feature before.
The main use case I've had for this has been when a tool's behaviour to find another tool or resource is to look for it in the PATH, so I need to configure the PATH environment variable to contain the directory of that tool's path.
Proposal A seems the most easy to understand and use but the flexibility of E is somewhat attractive too.

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.

Add $(location_dirname) template for resolving the directory of a label
6 participants