Page MenuHomePhabricator

[llvm-go] add component lib deps to llvm-go
AbandonedPublic

Authored by axw on Jan 20 2016, 5:40 PM.

Details

Reviewers
eugenis
pcc
Summary

llvm-go will call llvm-config --libs to get the
library names for all of the components required
to link Go code using the LLVM Go bindings. This
fails if the component libraries are not all built,
because llvm-go does not have the dependencies.

We move the dependencies off targets built with
llvm-go, and onto llvm-go itself, since llvm-go
is practically useless without them. This enables
running the Go bindings tests without building the
component libraries explicitly.

Diff Detail

Event Timeline

axw updated this revision to Diff 45476.Jan 20 2016, 5:40 PM
axw retitled this revision from to [llvm-go] add component lib deps to llvm-go.
axw updated this object.
axw added reviewers: eugenis, pcc.
axw added a subscriber: llvm-commits.
pcc edited edge metadata.Jan 20 2016, 6:01 PM

Does this solve the problem? In Evgeniy's configuration the libraries llvm-config was looking for wasn't being built (i.e. there were no build rules for them).

Can't you just append components on line 92 of llvm-go.go?

axw added a comment.Jan 20 2016, 6:12 PM
In D16387#331898, @pcc wrote:

Does this solve the problem?

With this change, check-llvm-bindings-go works in a clean build.

In Evgeniy's configuration the libraries llvm-config was looking for wasn't being built (i.e. there were no build rules for them).

For both libLLVMGlobalISel and libLLVMDebugInfoCodeView? The former would be lacking a build rule due it being optional, but was causing llvm-config to complain due to the component not being marked optional. I've fixed that in r258379.

Can't you just append components on line 92 of llvm-go.go?

I don't think that helps. As far as I can see, that list of components is only used by the standalone Go bindings build (bindings/go/build.sh).

The issue is that the lit tests are using llvm-go, but neither llvm-go nor the test targets currently depend on all of the components that it will use. I could add the dependencies to the test targets, but it seems a bit simpler just to have llvm-go depend on them. It's not much use without them.

pcc accepted this revision.Jan 20 2016, 6:24 PM
pcc edited edge metadata.

For both libLLVMGlobalISel and libLLVMDebugInfoCodeView? The former would be lacking a build rule due it being optional, but was causing llvm-config to complain due to the component not being marked optional. I've fixed that in r258379.

Okay, I see.

I don't think that helps. As far as I can see, that list of components is only used by the standalone Go bindings build (bindings/go/build.sh).

It used to be passed to llvm-config by llvm-go.go until your change r258283, and things worked until then. As you are fixing build breakage I would have preferred a more minimal change that reverts things to the way they used to work. But this is fine as well, LGTM.

This revision is now accepted and ready to land.Jan 20 2016, 6:24 PM
axw added a comment.Jan 20 2016, 6:31 PM
In D16387#331920, @pcc wrote:

For both libLLVMGlobalISel and libLLVMDebugInfoCodeView? The former would be lacking a build rule due it being optional, but was causing llvm-config to complain due to the component not being marked optional. I've fixed that in r258379.

Okay, I see.

I don't think that helps. As far as I can see, that list of components is only used by the standalone Go bindings build (bindings/go/build.sh).

It used to be passed to llvm-config by llvm-go.go until your change r258283, and things worked until then. As you are fixing build breakage I would have preferred a more minimal change that reverts things to the way they used to work. But this is fine as well, LGTM.

Oops, so it was. That was an accident. I'll just add components back in there, and forget about this change.

axw abandoned this revision.Jan 20 2016, 6:34 PM

Superceded by D16392