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

[WIP] Add support for cgo pkg-config #556

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
3 participants
@odinuge
Copy link

commented Jun 20, 2019

This fixes #203

The tests for pkg-config require libpng and libjpeg to be installed, and skips without failing if one of them isn't installed.

Signed-off-by: Odin Ugedal odin@ugedal.com

@odinuge odinuge requested review from ianthehat and jayconrod as code owners Jun 20, 2019

@odinuge odinuge force-pushed the odinuge:pkg-config branch 4 times, most recently from 37f7521 to c118745 Jun 20, 2019

Add support for cgo pkg-config
Signed-off-by: Odin Ugedal <odin@ugedal.com>

@odinuge odinuge force-pushed the odinuge:pkg-config branch from c118745 to 361a1fc Jun 21, 2019

@odinuge odinuge changed the title Add support for cgo pkg-config [WIP] Add support for cgo pkg-config Jun 21, 2019

@jayconrod

This comment has been minimized.

Copy link
Collaborator

commented Jun 21, 2019

Sorry, I don't think pkg-config can be reasonably supported. It fundamentally relies on system configuration outside the Bazel workspace. Flags appropriate for one system may not be appropriate for another. Different versions of a library may be installed (or not installed at all). Cross compilation and remote execution can't work.

A better way to depend on these libraries is to pull them in as git_repository or http_archive rules with build files that provide cc_library targets. Unfortunately, there's not an easy way to automate that.

@jayconrod jayconrod closed this Jun 21, 2019

@BenTheElder

This comment has been minimized.

Copy link

commented Jun 21, 2019

Can you elaborate on the better way @jayconrod? This is blocking Kubernetes from using a non-ancient version of libseccomp bindings.

@jayconrod

This comment has been minimized.

Copy link
Collaborator

commented Jun 21, 2019

@BenTheElder Sure, the key thing is that there needs to be a cc_library for the C/C++ dependency and go_library should depend on that via cdeps. cdeps must be added manually, but Gazelle will leave it alone.

Preparing the cc_library of course is the tricky part. There are a lot of ways to do that. I don't know enough about this library to know the best approach, but after a quick look at its configure.ac, it looks okay.

  • You could check in precompiled static or dynamic libraries for target platforms, then wrap them with a cc_import. cc_import can be used in place of a cc_library.
  • You could run ./configure && make -j in a genrule, then wrap the outputs in cc_import
  • It might be a simple enough library to write a cc_library or a handful of related cc_library rules.
@BenTheElder

This comment has been minimized.

Copy link

commented Jun 21, 2019

Thanks for the response, that sounds pretty involved :(

Fom some testing the library involved just needs a link flag to link the cgo to the library, pkg-config is not giving any other flags. Previously the go library configured that with a cgo directive but they switched to pkg-config (presumably for correctness?)

Kubernetes still checks all of its dependency code into vendor/ so I don't think this work around will work because it will cause gazelle to error out on parsing the code in vendor.

I'd be happy to manually patch in that link flag to clinkopts in the BUILD for the platforms we build the dependent binary (kubelet) on (Linux only..) and just ignore the pkg-config when building with bazel... in order to update the dependency but continue building as we did before.

Could we get a mode in gazelle to warn / skip / ignore pkg-config directives instead of failing?

@jayconrod

This comment has been minimized.

Copy link
Collaborator

commented Jun 21, 2019

Gazelle should just print a warning for pkg-config directives. From the code, it looks like the error is just passed to log.Printf. If it's failing and not generating anything, please open an issue.

After a warning though, manually setting clinkopts is probably okay but "at your own risk" :) Just make sure the library is there on the execution platform.

@BenTheElder

This comment has been minimized.

Copy link

commented Jun 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.