bazel-contrib / rules_go

Go rules for Bazel
Apache License 2.0
1.39k stars 662 forks source link

Provide ToRlocation() in runfiles package #3738

Open ashi009 opened 1 year ago

ashi009 commented 1 year ago

What version of rules_go are you using?

0.40.1

What version of gazelle are you using?

30.0

What version of Bazel are you using?

6.0

Does this issue reproduce with the latest releases of all the above?

Yes

What operating system and processor architecture are you using?

macOS 13.6 (22G120)

Any other potentially useful information about your toolchain?

What did you do?

What did you expect to see?

runfiles package only provides methods for resolve runfile paths to real ones, but doesn't have a way to generate a runfile path, i.e. rlocationpath.

At this point, aspect bazel-lib provides to_rlocation_path function for doing this in skylark, would be nice to have this function in native go.

What did you see instead?

fmeum commented 1 year ago

The bazel-lib function operates on Starlark File functions representing build artifacts tracked by Bazel, which have no equivalent in Go (or any other language). What is your use case?

ashi009 commented 1 year ago

I have two rules, the first one generates a bunch of k8s config files, and the second one runs with those files.

As there are import statements in the generated files, the second application needs to be able to find those files in runfiles. An easy and reliable approach would be generate those import statements with runfile paths directly.

fmeum commented 1 year ago

I agree that runfiles paths would make sense as imports. But couldn't you just pass a mapping from full paths to runfiles paths into the first action, using the existing Starlark function? That seems cleaner to me than reverse engineering the runfiles path from a full path in Go, which would necessarily depend on Bazel implementation details.

ashi009 commented 1 year ago

Doable, but it will double the flags passed to the first action.

fmeum commented 1 year ago

Yes, that's true, but at least this can be handled quite nicely with ctx.actions.args map_each on the Starlark side.

Going from full path to runfiles path can be pretty tricky (take a look at the Go or Bash runfiles library's internals) and may not even always be possible unambiguously, especially when different Bazel versions are involved. I don't think we can reasonably commit to providing that functionality, especially not if there is a conceptually much cleaner alternative.

ashi009 commented 1 year ago

The skylib implementation uses short_path for this, instead of the full path. Which is like a 2 line function. The problem with map_each here is that the skylib implementation requires ctx.workspace_name to generate the runfile path, and per the doc

To avoid unintended retention of large analysis-phase data structures into the execution phase, the map_each function must be declared by a top-level def statement; it may not be a nested function closure by default.

which mean there is no way to pass ctx.workspace_name to the function. We might be able to get around it with a manual loop. But this will not work when there are directory outputs...

fmeum commented 1 year ago

It's only a 2 line function because Bazel knows more about a given file than just its path though.

I tried before but unfortunately building a general purpose rlocation_path field on File is pretty difficult. See https://github.com/bazelbuild/bazel/issues/14307#issuecomment-1060720055 for a workaround (which you can make work for the main repository by using the function from bazel-lib in map_each).