Skip to content

feat: add support for pkg-config #2534

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

Closed
wants to merge 1 commit into from

Conversation

rockstar
Copy link
Contributor

This patch adds support for pkg-config, so that when go build is
invoked, libflux is built as part of finding the path to the library.

This patch adds support for `pkg-config`, so that when `go build` is
invoked, `libflux` is built as part of finding the path to the library.
@rockstar rockstar requested a review from a team February 19, 2020 19:40
@ghost ghost requested review from fchikwekwe and removed request for a team February 19, 2020 19:41
Comment on lines +5 to +6
// #cgo pkg-config: flux
// #include "influxdata/flux.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really understand these cgo declarations. Did we move this header file?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see above that it was deleted. Still wondering about the way that the naming here changed. Why the influxdata/flux.h instead of just referencing the file. Does it live somewhere else now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There were two flux.h files (I seem to remember that it's actually just a symlink to the original), and I can't remember the reasoning for it, other than it was a hack to help link the rust from go. With pkg-config, we don't need to do any weird symlinking or keep copies of files around; the hacks can go away.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I saw that you were able to remove the symlinks with this change. That's awesome. The symlinks were a little confusing and cumbersome to me.

Comment on lines +18 to +23
To build flux, first install the `pkg-config` utility, and ensure the GNU `pkg-config` utility is also installed.

```
$ go get github.com/influxdata/pkg-config
```

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for including this. Reading a bit of the pkg-config information helped with understanding some of these changes.

@fchikwekwe
Copy link
Contributor

Just a few questions for my own understanding, but overall this looks good to me.

@rockstar
Copy link
Contributor Author

@wolffcm re-targeted this against feat/use-algo-w while I was away (which was what I planned to do), so it's no longer necessary here.

@rockstar rockstar closed this Feb 28, 2020
@wolffcm
Copy link

wolffcm commented Feb 28, 2020

This was the PR that landed pkg-config builds on the algo-w branch: #2555

@rockstar rockstar deleted the feat/pkg-config-support branch April 7, 2020 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants