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

go_binary output paths contain a spurious os_arch component #1239

Closed
creachadair opened this issue Jan 13, 2018 · 28 comments · Fixed by #1393
Closed

go_binary output paths contain a spurious os_arch component #1239

creachadair opened this issue Jan 13, 2018 · 28 comments · Fixed by #1393

Comments

@creachadair
Copy link

creachadair commented Jan 13, 2018

Building a go_binary results in an output path that differs from the package path by the addition of $GOHOSTENV, $GOOS, and $GOARCH, e.g.,

bazel build //path/to/my:binary

leaves the binary at

bazel-bin/path/to/my/linux_amd64_stripped/binary

(or whatever your os/arch/etc. works out to be)

This makes it difficult to use Go binaries as part of sh_test rules: Other Bazel *_binary tools can be used by relative path without having to pre-bake these values into the script, e.g., if my sh_binary declares //path/to/my:binary as a data dependency, it should be executable by the path path/to/my/binary relative to the exec root when the test runs.

It works as described for everything but Go. Given that the Bazel output already reflects host and architecture constraints, the additional path decoration gets in the way of common practice.

One can work around this by injecting rewrite rules, but that shouldn't be necessary.

@jayconrod
Copy link
Contributor

cc @ianthehat

We added this to avoid cache poisoning when building the same targets in different modes. For example, if you ran a bunch of tests in race mode, then build a binary that depends on the same libraries as the tests, all the libraries have to be rebuilt. We work around this by including mode information in the output paths, which allows Bazel to cache files in multiple modes.

Other Bazel *_binary tools can be used by relative path without having to pre-bake these values into the script

Is there another set of Skylark rules that work this way? The C / C++ / Objective C rules may have more flexibility than we do since they're part of Bazel itself.

@creachadair
Copy link
Author

We added this to avoid cache poisoning when building the same targets in different modes.

I'm not sure what you mean by "modes" in this context, but it seems to me like the mode should land in the build configuration, thus changing (say) k8-fastbuild to k8-fastbuild-race or similar just like -c opt and -c fastbuild do.

@ianthehat
Copy link
Contributor

The way bazel works, that part of the path is constant for any given build, and not configurable in any way. It's affected by the existing options only (so we could not add race to it for instance). We also support building the same binary in multiple modes within a single build right now.
When bazel gets controllable toolchain transitions, and the active toolchain causes the root of the path to change, we will probably take it out of our generated path, but even then it does not help you.

If you hard code the path in a sh_test it's going to be wrong on windows (or any other platform where the binary name does not match the rule name). The same is true for all the languages, it's never safe to make this assumption. For the most common languages (c and, java), on linux and mac, the assumption holds, but it's not a good idea to rely on it.
You should always be using $(location :target) to find out file names, never assuming that it's some standard transformation of the rule name.

This is documented in the FAQ

@jmillikin-stripe
Copy link
Contributor

jmillikin-stripe commented Jan 19, 2018

I just hit this too -- it's pretty frustrating to deal with, considering that it seems to be unique among per-language _binary rules.

Since the cache is per-target, would it be possible to leave the arch in the library file names, but have binary names return to archless? Then most of the build would be cached, and the only actions that need to re-run on a compilation mode change are the final link steps.

@jmmv
Copy link
Contributor

jmmv commented Jan 23, 2018

Another +1 to this issue. I just upgraded a project to 0.9.0 (from 0.7.0) and all my support tools and tests started failing because they could no longer find the binary artifacts declared as data dependencies. I find this very confusing because no other rule that I know of behaves this way: building //foo:bar results in ./bazel-bin/foo/bar being runnable.

The suggestion above to use $(location ...) works for tests (in a pretty convoluted manner), but it cannot be made to work for other kinds of rules. Plus this makes it hard for a script wrapping Bazel to find the location of the built artifacts.

@creachadair
Copy link
Author

Most sh_test scripts are not written to plumb through the paths. When working around this, I considered two approaches: One is to provide each binary with a path-rewriting rule, that copies the output to the path the script expects. The other (which I implemented here) was to wrap the sh_test in a macro that shoves the paths in via variables defined at the top of the script.

I chose the latter even though it forced me to tweak the scripts a bit, because the former wound up being a lot more code and required a rule instead of just a macro. Clearly either approach could have worked, though.

The other problem I ran into with plumbing $(location ...) through was that it's difficult to apply consistently across languages. For example: Naively, you could give the script a variable definition like

export MYTOOL=$(location //path/to/some:tool)

But that fails for java_binary rules, which produce multiple outputs ($(location ...) gives an error, and $(locations ...) produces an invalid script). This too can be solved, but a principled solution is a lot more complex. In the end, I took the more expedient approach of not rewriting the paths for Java tools—hopefully those will be more stable since they're native rather than Skylark.

It seems fairly clear that the design of sh_test did not take this into account, even though it's in principle always been possible for binary dependencies not to match their rule paths. Regrettably, faced between a trade-off between Bazel having broken the behaviour our old (pre-bazel_go) Go rules relied on, and bazel_go having broken the behaviour our sh_test rules relied on, I chose to address the latter. It felt a lot like running to stand still, though.

@ianthehat
Copy link
Contributor

I agree that it's not nearly as easy to use $location and friends as it should be, and that bazel could do with some improvements here.

If we can come up with a good proposal I am happy to push the Bazel team to accept it, but it would probably have to be something much more general dealing with runfiles in all targets.

Maybe the data attribute could accept some kind of mapping directive and bazel would automatically set an environment variable of that name to the location of the file.

@creachadair
Copy link
Author

What about @jmillikin-stripe's proposal?

@jmmv
Copy link
Contributor

jmmv commented Jan 26, 2018

What about supporting $(location) in x_defs? I'm in the need of hardcoding locations to dependent binaries within another binary, and this could make things much easier. Passing the value as an argument does not help, because I want to be able to do bazel run ... on the corresponding go_binary rule and have things just work.

(This doesn't resolve the issue of not being able to deterministically invoke built binaries outside of Bazel... which is very annoying and inconsistent because all other binary rules support this.... but oh well.)

@rogerhub
Copy link

rogerhub commented Jan 27, 2018

If you're using bazel in scripts, what about the Build Event Protocol? It lets you programmatically find the output files generated by your builds, and it's probably more robust than hard-coding paths relative to bazel-bin.

@creachadair
Copy link
Author

bazel run should work regardless—it knows how to extract the correct executable path from the rule outputs. (Separately, I think bazel run is almost never the right way to invoke a tool, because it screws up the environment in other ways—but that's a separate issue from the path)

If you're using bazel in scripts, what about the Build Event Protocol?

I don't follow how that would help in this case. How is a script supposed to get information from the protocol without explicitly rebuilding all its tools inline?

@rogerhub
Copy link

@creachadair Whoops, I don't have a good answer for that. I was trying to address "This doesn't resolve the issue of not being able to deterministically invoke built binaries outside of Bazel" (I'm assuming for CI/CD).

@jmmv
Copy link
Contributor

jmmv commented Jan 29, 2018

@rogerhub Yes... there can be ways to figure out the name of a binary, but using the BEP to find the location of an artifact? This is like using a cannon to kill flies. If you have a build tool and you want to build something, there better be a simple way to access the build results. Not everybody is going to use bazel run and bazel test exclusively: people will want to access the build artifacts to... install them? And that's pretty trivial with (all?) other build systems, and impossible now with this change.

I personally find this whole thing a bit ridiculous. Per the first reply in this bug, this breaking change was to "avoid cache poisoning when building the same targets in different modes." but... all Bazel rules are affected by this problem, aren't they? Therefore, it's not the Go rules' job to fix this: it's Bazel who should fix it. And unless Bazel fixes it, being different here is just adding a different source of pain that no other rules expose. (Note that the whole point of the bazel-bin et. al. symlinks is to hide the fact that builds generate artifacts for different configurations.)

The reason I'm calling this ridiculous is because working around this change is extremely hard and brittle. See bazelbuild/sandboxfs#13 for the changes I had to apply to a project... and they are far from nice. I'm at the point of questioning whether going the Bazel route was a good idea at all.

@ianthehat
Copy link
Contributor

I'm not really sure why this is such a big deal, in the cases where you know for sure you are never going to have a colliding binary, the genrule workaround is just

genrule(
    name = "normalised_binary",
    srcs = [":some_binary"],
    outs = ["the_binary"],
    cmd = "cp $(SRCS) $@",
)

and then you have a dependable binary filename ready to go.
There is no workaround for people that do not want binaries to collide if I go back to the previous method.
If it's really common that people want to do this, and it's really to much to add the genrule in the cases where it is needed, we can talk about adding an option back to go_binary, but I really don't want to encourage this, it's the wrong answer in most cases, and if you point me at specific examples I will happily tell you what I think the right answer is and possible add another example of doing it to rules_go.

In the mean time I am adding an example of using this approach in #1286.

Not all rules are affected, because very few of the other languages have modes that can be controlled the way the go rules do. If you are specifying it from the command line, and the execution root is changing, then you don't have this problem.
When Bazel gets around to adding the ability to dynamically control the platform we will switch to that mechanism, but at the same time all the other languages will break in the same way, because now you will be able to write a rule that depends on both the windows and linux executables, which means they both need to be present under a single execution root and thus the filename presented to the rules must include that part again.
Also, when we add the ability to compile a go_binary as a plugin or a c shared library, the extension is going to depend on the build mode as well, I totally agree we need better ways of doing the $(location) expansion from the Bazel team but relying on specific filenames is not a scalable or sustainable answer.

@jmmv
Copy link
Contributor

jmmv commented Jan 29, 2018

Please look at the PR I referenced in the previous comment. There is more going on than the genrule you propose (which, if sufficient, I'd trivially hide behind a Skylark macro as proposed in bazelbuild/bazel-gazelle#99, which just requires a minor Gazelle extension).

In particular: how do you tell users to run a binary that they have just built? bazel run is not an option because the binary has to be run with sudo, and merging sudo with a Bazel invocation is not a great idea.

Related to the previous point: how can a program locate its runfiles if it's executed outside of bazel run? In the past this was easy because you knew the location of the directory the runfiles were in. Now that's not true any longer. (Maybe the genrule could be extended to also copy the runfiles. I gave this a try but couldn't get it to work; can't remember now.)

@ianthehat
Copy link
Contributor

I don't understand why the genrule is not sufficient.
It converts from a path you don't know to a path you do know, and that's it. You depend on and use the genrule output instead of the go_binary one. You tell users to build and then run the genrule, not the go_binary rule.
I did look at your PR, and it seemed to me if you renamed the go_binary rule, and then added the genrule to copy from that and called it install, it would have solved the problem with no other changes needed. And yes, you could probably wrap the pair into a single macro.

Locating your runfiles from outside of bazel-run is generally an unsafe idea. There is no guarantee that the path inside the runfiles will be the same as the one the user might see through the symlinks. Especially as the user is allowed to rename or suppress the symlinks. You are making a binary that may be referring to the wrong things, or missing things, and it's going to hang around for ever, some user is going to copy it out to their bin directory so they can run it easily and it's going to cause obtuse bugs in the long run.

Instead you add a packaging rule to put everything into a single file, possibly a self installing archive, and produce that for the user to run. This is clean and safe, and produces a single file you can give to other people, or hold on to and run again without depending on your build state.

You can also do this by making a single go binary with all the things that would be in runfiles as embedded data, which is slightly more convoluted but is really clean and has no extra dependencies.

@jmmv
Copy link
Contributor

jmmv commented Jan 29, 2018

Well, I can't apply the genrule trick unless Gazelle is allowed to have different rule names or unless I stop using Gazelle... and the latter would be sad. So maybe that's the real problem and should be prioritized?

@ianthehat
Copy link
Contributor

Yes, gazelle needs to be able to cope with non standard binary names.
Filed bazelbuild/bazel-gazelle#106

@jayconrod
Copy link
Contributor

I'll try to figure out a matching policy that makes sense to fix bazelbuild/bazel-gazelle#106 tomorrow.

I want to be careful here, especially in situations where there are multiple go_binary or go_library rules in the same build file.

@creachadair
Copy link
Author

I would recommend you also consider updating https://docs.bazel.build/versions/master/build-ref.html#data, which currently says

These files are available using the relative path path/to/data/file.
In tests, it is also possible to refer to them by joining the paths
of the test's source directory and the workspace-relative path,
e.g. ${TEST_SRCDIR}/workspace/path/to/data/file.

This is currently false when using the Go rules.

@ianthehat
Copy link
Contributor

That is talking about data files that are present in the source folder, generated files don't have a relative to workspace-relative path because they are not in the workspace...

@creachadair
Copy link
Author

Nothing about that section specifies source files. Just above, it specifically calls out that this applies to rules, not just files:

if a binary/library/test needs some files to run, specify them (or a build rule containing them) in data

Granted, a rule could just be a filegroup, but that is at best a misleading assumption, since nothing in that section stipulates constraints.

@jmmv
Copy link
Contributor

jmmv commented Feb 7, 2018

Coming back to this because I still haven't been able to workaround this problem.

Turns out the genrule trick is insufficient because it cannot deal with the runfiles tree: once the genrule declares more than one output (binary + runfiles), it cannot be declared as executable any more and thus is not usable with bazel run. And if it does not handle the runfiles, then it's incomplete.

The question is: if you have a go_binary that wants to execute another go_binary as a helper tool... what's the work-around?

jmmv added a commit to jmmv/sandboxfs that referenced this issue Feb 8, 2018
This change adds a bunch of Bazel-specific functions to locate the runfiles
trees of built binaries and to find binaries within them, and then changes
the lint and install tools to use these new features.

This extra logic is required to later deal with a change in the Go rules
that causes the paths to built Go binaries to not be deterministic
(bazelbuild/rules_go#1239).  Needless to mention
that this has been a huge time sink.
jmmv added a commit to jmmv/sandboxfs that referenced this issue Feb 8, 2018
As part of this change, rerun Gazelle to update all BUILD.bazel files
with new changes.

We also have to workaround a change in the paths of the built
Go binaries (bazelbuild/rules_go#1239) in our
instructions and build scripts.
jmmv added a commit to jmmv/sandboxfs that referenced this issue Feb 9, 2018
As part of this change, rerun Gazelle to update all BUILD.bazel files
with new changes.

We also have to workaround a change in the paths of the built
Go binaries (bazelbuild/rules_go#1239) in our
instructions and build scripts.
jmmv added a commit to jmmv/rules_go that referenced this issue Feb 16, 2018
This change adds a bunch of Bazel-specific functions to locate the
runfiles trees of built binaries and to find binaries within them.

This extra logic is required to support the recent change in the Go
rules that causes the paths to built Go binaries to not be deterministic
(issue bazelbuild#1239).
jayconrod pushed a commit that referenced this issue Feb 16, 2018
This change adds a bunch of Bazel-specific functions to locate the
runfiles trees of built binaries and to find binaries within them.

This extra logic is required to support the recent change in the Go
rules that causes the paths to built Go binaries to not be deterministic
(issue #1239).
@jmmv
Copy link
Contributor

jmmv commented Feb 16, 2018

FTR #1331 has added a bunch of utility functions to the go/tools/bazel package to locate the runfiles tree of a binary and to find other binaries within it.

jmmv added a commit to jmmv/sandboxfs that referenced this issue Feb 16, 2018
Change the lint and install tools to use the APIs in the Go rules package
to find their runfiles trees and built binaries within them.

This extra logic is required to later deal with a change in the Go rules
that causes the paths to built Go binaries to not be deterministic
(bazelbuild/rules_go#1239).
jmmv added a commit to jmmv/sandboxfs that referenced this issue Feb 16, 2018
As part of this change, rerun Gazelle to update all BUILD.bazel files
with new changes.

We also have to workaround a change in the paths of the built
Go binaries (bazelbuild/rules_go#1239) in our
instructions and build scripts.
jayconrod pushed a commit to jayconrod/rules_go that referenced this issue Mar 20, 2018
go_binary now has an "out" attribute which allows users to set the
output filename for the generated executable. When set, go_binary will
write this file without mode-specific directory prefixes, without
linkmode-specific prefixes, and without platform-specific extensions.

Fixes bazelbuild#1239
jayconrod pushed a commit to jayconrod/rules_go that referenced this issue Mar 20, 2018
go_binary now has an "out" attribute which allows users to set the
output filename for the generated executable. When set, go_binary will
write this file without mode-specific directory prefixes, without
linkmode-specific prefixes, and without platform-specific extensions.

Fixes bazelbuild#1239
jayconrod added a commit that referenced this issue Mar 21, 2018
go_binary now has an "out" attribute which allows users to set the
output filename for the generated executable. When set, go_binary will
write this file without mode-specific directory prefixes, without
linkmode-specific prefixes, and without platform-specific extensions.

Fixes #1239
@jmillikin-stripe
Copy link
Contributor

I know this was closed, but in case anyone else continues to be annoyed by go_binary output paths, #2249 is another attempt to make it configurable.

@alexeagle
Copy link
Contributor

dropping this here in case others find it useful. bazel aquery queries the action graph without executing it, and knows what outputs are declared by go_binary. We know the mnemonic that produces the final binary is GoLink, so we "just" need to parse the output of aquery a bit.

For compat with bazel 3 and 4, we need --noincompatible_proto_output_v2

Since the proto output of aquery has the bits spread around, the jq filter is kinda fancy. In a real language this could be implemented more cleanly...

$ bazel 2>/dev/null aquery --noincompatible_proto_output_v2 //:example-go --include_commandline=false --output jsonproto | jq -r '((.actions | map(select(.mnemonic == "GoLink"))[]?.outputIds)[] as $ids | (.artifacts[] | select(.id | IN( $ids )))) as $art | $art.execPath'

bazel-out/k8-fastbuild/bin/example-go_/example-go

@sluongng
Copy link
Contributor

@alexeagle The jq query there does not work for me in Bazel 5.1.0

I got to this

jq '.artifacts[] | select(.id | IN(308))'
{
  "id": 308,
  "pathFragmentId": 686
}

which could not be assigned to $art.execPath. Instead you have to dive into the .pathFragments[] array and find the matched node, which only give you the file name.

jq '.pathFragments[] | select(.id == 686)'
{
  "id": 686,
  "label": "my_binary.exe",
  "parentId": 687
}

Then you have to find the full path by keep recursively traverse upward via parentId of the pathFragment.


I think the genrule trick mentioned in this thread worked for me. My use case is that I have a cross compiled (from linux/macos) window binary that I need to get it's output path to copy else where. Here is what I have

...

go_binary(
    name = "my_binary",
    embed = [":go_default_library"],
    goarch = "amd64",
    goos = "windows",
    tags = ["windows"],
    visibility = ["//visibility:public"],
)

genrule(
    name = "binary_path",
    srcs = [":my_binary"],
    outs = ["echo_binary_path.sh"],
    cmd = """
echo '#!/bin/bash
echo $(location :my_binary)' > $@
    """,
    executable = True,
)

and doing bazel run :binary_path would get me the output path needed.

Worth to note that the binary needs to be in srcs attribute and not tools attribute of the genrule. Using tools attribute would result in the binary being compiled using host's toolchain instead.

@fmeum
Copy link
Collaborator

fmeum commented Jul 29, 2022

With bazelbuild/bazel#8739 fixed in Bazel 5.3.0 and current Bazel HEAD, you can instead do (example uses a go_binary from the rules_go tests):

$ bazel cquery --output=files //tests/core/go_binary:hello
bazel-out/k8-fastbuild/bin/tests/core/go_binary/hello_/hello

If you need to get the path from within a Bazel build, I would recommend writing a small custom rule that takes the go_binary target in as an attribute. The rule than has full access to path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
9 participants