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

Clarify external use and support for pkg/bindings #6866

Closed
drewbailey opened this issue Jul 6, 2020 · 36 comments
Closed

Clarify external use and support for pkg/bindings #6866

drewbailey opened this issue Jul 6, 2020 · 36 comments
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.

Comments

@drewbailey
Copy link

/kind feature

Clarify support & usage of pkg/bindings

pkg/bindings seems like current and only go client to interact with the podman http api. It would be reassuring if this package was given a readme and identified as a package that can and should be consumed by external projects similar to docker's go client https://github.com/moby/moby/tree/master/client

I was also wondering if there had been any consideration of turning this into it's own go module to ease external adoption.

@openshift-ci-robot openshift-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jul 6, 2020
@github-actions
Copy link

github-actions bot commented Aug 6, 2020

A friendly reminder that this issue had no activity for 30 days.

@mheon
Copy link
Member

mheon commented Aug 6, 2020

@rhatdan @baude @jwhonce We should discuss this one at scrum and decide what to do. I'm not opposed to splitting this out, but we probably want to wait until the libpod move to do it (to avoid having something that vendors containers/podman, and is then vendored into containers/podman...)

@baude
Copy link
Member

baude commented Aug 6, 2020

@drewbailey as @mheon suggested, we did discuss this as a team. the overwhelming response was to keep to the bindings in containers/podman. however, we do agree that adding readme's and other related things are more than appropriate. we have a tutorial coming out here very shortly from a team member on how to consume and use the bindings. from that, we will derive a readme.

i am wondering ... if you guys have started using the bindings, do you want to contribute a readme or anything related?

@drewbailey
Copy link
Author

drewbailey commented Aug 10, 2020

@baude sorry, I should have been more specific. I don't think many users would care if the package lived in containers/podman as a submodule or some other repository. But, in it's current form, I don't think that pkg/bindings is suitable for community use. Since it is currently not isolated from the rest of the podman project, community projects will need to specify the entirety of the podman project as a dependency to use pkg/bindings.

Consider a simple main.go

package main

import (
	"context"

	"github.com/containers/libpod/v2/pkg/bindings"
)

func main() {
	_, err := bindings.GetClient(context.Background())
	if err != nil {
		panic(err)
	}
}

Attempting to fetch the package leads to the following errors which can be resolved by installing a few packages.

λ drew [podman-test] → go get github.com/containers/libpod/v2/pkg/bindings
go: found github.com/containers/libpod/v2/pkg/bindings in github.com/containers/libpod/v2 v2.0.4

# pkg-config --cflags  -- devmapper
Package devmapper was not found in the pkg-config search path.
Perhaps you should add the directory containing `devmapper.pc'
to the PKG_CONFIG_PATH environment variable
No package 'devmapper' found
pkg-config: exit status 1
# github.com/mtrmac/gpgme
../../../go/pkg/mod/github.com/mtrmac/[email protected]/data.go:4:11: fatal error: gpgme.h: No such file or directory
    4 | // #include <gpgme.h>
      |           ^~~~~~~~~
compilation terminated.
# github.com/containers/storage/drivers/btrfs
../../../go/pkg/mod/github.com/containers/[email protected]/drivers/btrfs/btrfs.go:8:10: fatal error: btrfs/ioctl.h: No such file or directory
    8 | #include <btrfs/ioctl.h>
      |          ^~~~~~~~~~~~~~~
compilation terminated.

The resulting go.mod

module github.com/drewbailey/podman-test

go 1.14

require github.com/containers/libpod/v2 v2.0.4

This alone adds around 34mb to the apps vendor directory.

For now, we will just be writing our own client, but we would definitely be interested in a community go client to interact with the api, much like Docker does with https://github.com/moby/moby/tree/master/client

@rhatdan
Copy link
Member

rhatdan commented Aug 10, 2020

I think it would be better for us to fix this exact behaviour then to break it out. A lot easier for us in the long run.

@mheon
Copy link
Member

mheon commented Aug 10, 2020

We probably need to audit the imports of pkg/bindings and figure out what we're dragging in... It could be we're using a random structure out of Kubernetes or CNI and dragging in massive packages from those two as a result.

@vrothberg
Copy link
Member

Based on the error it looks like pkg/bindings is importing c/storage and c/image. Those will be pulled wherever the package is, so we need to prune the dependencies and add checks to the CI to prevent them from being pulled back.

@rhatdan
Copy link
Member

rhatdan commented Sep 11, 2020

@baude I think after the bug week, we should work on this. Basically figure out how we can minimize the stuff we pull in for bindings. Something you are pretty good at. :^)

@baude
Copy link
Member

baude commented Oct 7, 2020

@vrothberg lets discuss tomorrow morning. I don't think this appears to be what we speculated... module analysis provided below.

3.6M runtime
1.4M reflect
828K syscall
468K time
424K regexp/syntax
416K internal/reflectlite
400K regexp
400K os
352K image
276K strings
276K strconv
240K internal/poll
228K math
224K unicode
208K bytes
200K encoding/binary
176K bufio
160K sync
144K crypto/cipher
132K io
116K sort
112K math/rand
108K context
104K path/filepath
88K image/color
76K hash/fnv
76K encoding/base64
72K io/ioutil
60K hash/crc32
60K container/list
52K internal/fmtsort
44K math/bits
40K github.com/containers/podman/vendor/golang.org/x/crypto/openpgp/errors
36K internal/cpu
28K unicode/utf8
28K errors
28K crypto
24K sync/atomic
20K runtime/internal/sys
20K internal/bytealg
20K image/internal/imageutil
16K runtime/internal/atomic
16K internal/testlog
16K internal/syscall/unix
16K hash
16K github.com/containers/podman/vendor/github.com/containers/image/v5/pkg/compression/internal
16K crypto/hmac
8.0K internal/race
8.0K crypto/subtle
4.0K vendor/golang.org/x/crypto/internal/subtle
4.0K runtime/internal/math
4.0K internal/syscall/execenv
4.0K internal/oserror
4.0K github.com/containers/podman/vendor/golang.org/x/sys/internal/unsafeheader
4.0K github.com/containers/podman/vendor/github.com/containers/common/version
4.0K crypto/internal/subtle

@vrothberg
Copy link
Member

Running ./dependencies/analyses/dependency-tree.sh github.com/containers/podman/v2/pkg/bindings lists a lot more (transitive) dependencies, including containers/storage, image and so on.

@edigaryev
Copy link
Contributor

I'm currently investigating options to integrate Podman support into Cirrus CLI and the biggest issue so far is that Podman's package transitively depends on GPGME's C headers, coming from https://github.com/containers/image, which uses https://github.com/mtrmac/gpgme by default.

This was discussed multiple times to no avail:

@edigaryev
Copy link
Contributor

Turns out that GPGME is not the only C-thing that gets pulled.

On Linux, https://github.com/containers/storage tries to link against btrfs by default and fails if no library and headers are installed.

And since there are a lot more build tags in the Podman's installation docs, I'm assuming this is just the tip of the iceberg.

It would be nice if one could control build tags of the dependencies from go.mod, but the seemingly related issue in the Go's bug tracker is still unresolved: golang/go#25873.

@rhatdan
Copy link
Member

rhatdan commented Nov 5, 2020

We have talked about making this better, but it comes down to priorities. Currently this has not bubbled up, which means we probably need community to take the lead on this.

@edigaryev
Copy link
Contributor

We have talked about making this better, but it comes down to priorities. Currently this has not bubbled up, which means we probably need community to take the lead on this.

Is there a consensus on what needs to be done to move this forward?

@rhatdan
Copy link
Member

rhatdan commented Dec 24, 2020

@baude you have been working on this. What is the current state?

@github-actions
Copy link

A friendly reminder that this issue had no activity for 30 days.

@rhatdan
Copy link
Member

rhatdan commented Jan 25, 2021

@baude Any progress?

@baude
Copy link
Member

baude commented Jan 25, 2021

I did create a readme for the bindings -> https://github.com/containers/podman/blob/master/pkg/bindings/README.md

as for the reduction in dependencies, you know where we stand on that. it is a longer work item given the package deps that exist.

@baude
Copy link
Member

baude commented Jan 29, 2021

I'm going to close this issue. The readme has been published. The effort to reduce tthe dependency chain on the bindings is important to us and it is being tracked in a different place for the team. It, however, is going to be a longer term effort crossing multiple git repos. We'll get it done!

@baude baude closed this as completed Jan 29, 2021
@drewbailey
Copy link
Author

@baude can you link the dependency issue? Unfortunately I think it will be challenging for community projects to adopt pkg/bindings to interact with the podman http api until that work is complete.

Has there been any discussion around a separate package for using the podman http api? similar to https://github.com/moby/moby/tree/master/client

@baude
Copy link
Member

baude commented Feb 15, 2021

Moving to a separate repo will still pull in all the dependencies that are present in the main repo.

@drewbailey
Copy link
Author

If it's purely a lift and shift, sure. I think the community would greatly benefit from a widely adopted and easy to use api package, similar to https://github.com/moby/moby/tree/master/client

@owais
Copy link

owais commented Oct 6, 2021

FWIW, this caused a lot of pain for us over at OpenTelemetry when we tried to add support for Podman monitoring to the point that we ended up ditching the official bindings and using the REST API instead duplicating quite a bit of code from the bindings. It would be great if we could have the bindings published as an independent Go module without depending on the podman project itself.

open-telemetry/opentelemetry-collector-contrib#5244
open-telemetry/opentelemetry-collector-contrib#5430

@vsoch
Copy link
Contributor

vsoch commented Dec 23, 2021

hey y'all, sorry to be the lemur to comment on a closed issue, but I'm developing on Ubuntu 20.04 and just hit this issue when trying to add podman as a Go module:

...
to the PKG_CONFIG_PATH environment variable
No package 'devmapper' found
pkg-config: exit status 1
github.com/mtrmac/gpgme
# github.com/containers/storage/drivers/btrfs
../../../../go/pkg/mod/github.com/containers/[email protected]/drivers/btrfs/btrfs.go:8:10: fatal error: btrfs/ioctl.h: No such file or directory
    8 | #include <btrfs/ioctl.h>
      |          ^~~~~~~~~~~~~~~
compilation terminated.
# github.com/mtrmac/gpgme
../../../../go/pkg/mod/github.com/mtrmac/[email protected]/data.go:4:11: fatal error: gpgme.h: No such file or directory
    4 | // #include <gpgme.h>
      |           ^~~~~~~~~
compilation terminated.
# github.com/containers/image/v5/manifest
../../../../go/pkg/mod/github.com/containers/image/[email protected]/manifest/oci.go:44:234: undefined: v1.MediaTypeImageLayerNonDistributableZstd
../../../../go/pkg/mod/github.com/containers/image/[email protected]/manifest/oci.go:44: undefined: v1.MediaTypeImageLayerZstd
../../../../go/pkg/mod/github.com/containers/image/[email protected]/manifest/oci.go:105:39: undefined: v1.MediaTypeImageLayerNonDistributableZstd
../../../../go/pkg/mod/github.com/containers/image/[email protected]/manifest/oci.go:110:39: undefined: v1.MediaTypeImageLayerZstd
make: *** [Makefile:14: all] Error 2

I understand it's a hard problem, but for developers (and eventual users) I'm not happy with trying to add a Go module (which usually works out of the box) and seeing an error traceback like this, and then being faced with "Oh no, how do I workaround this?" I suspect the solution is something I observed a little earlier tonight - to place the burden of install on the user and just interact with Podman via shell from Go: https://github.com/containers/toolbox/blob/73c53a347b402748b6a12ef9f74cecb7e8528c38/src/pkg/podman/podman.go#L59

So here I am posting on a closed issue because I want to express that the current state isn't ideal. Is there absolutely no means for an automated approach so I can add it as a Go module and rest assured that it's going to build for others (and future me?) Apologies for my naivete I haven't looked closely into the issue, but it seems like two missing dependency libraries, and I would really like for this to work! I suspect I'm going to do the same for the time being as the link above - place the burden of installing Podman on the user and just interacting via command line.

@rhatdan
Copy link
Member

rhatdan commented Dec 23, 2021

You need to set the BUILDTAGS to turn this off.

@rhatdan
Copy link
Member

rhatdan commented Dec 23, 2021

@mtrmac @nalind Should we change these to required Buildtags to add versus defaulting them on and requiring users to turn them off.

@vsoch
Copy link
Contributor

vsoch commented Dec 23, 2021

I think that would be a good solution so the build works out of the box.

@mtrmac
Copy link
Collaborator

mtrmac commented Jan 3, 2022

In the general case of insufficient build automation, build instructions, and/or attention being paid to build instructions, I strongly prefer a default that has better security, and better features (using a FIPS-validated, non-Go, actually maintained, signature implementation, that can sign) over a default that saves a a few (dozen?) megabytes.

(Of course the real fix would be to fix the dependencies so that this tradeoff doesn’t come up, but when it does come up, the choice just seems so clear to me.)

@hellt
Copy link

hellt commented Jan 14, 2022

You need to set the BUILDTAGS to turn this off.

Hi @rhatdan
which buildtags will disable the following errors?

# github.com/containers/image/v5/manifest
../../../../go/pkg/mod/github.com/containers/image/[email protected]/manifest/oci.go:44:234: undefined: v1.MediaTypeImageLayerNonDistributableZstd
../../../../go/pkg/mod/github.com/containers/image/[email protected]/manifest/oci.go:44: undefined: v1.MediaTypeImageLayerZstd
../../../../go/pkg/mod/github.com/containers/image/[email protected]/manifest/oci.go:105:39: undefined: v1.MediaTypeImageLayerNonDistributableZstd
../../../../go/pkg/mod/github.com/containers/image/[email protected]/manifest/oci.go:110:39: undefined: v1.MediaTypeImageLayerZstd

@rhatdan
Copy link
Member

rhatdan commented Jan 14, 2022

I have no idea. @mtrmac @giuseppe PTAL

@hellt
Copy link

hellt commented Jan 14, 2022

@rhatdan I got a message from my colleague, that he fixed it with pinning the image-spec module version srl-labs/containerlab@509ff34

@hellt
Copy link

hellt commented Jan 14, 2022

so it seems containerd 1.5.9 has image-spec 1.2.0 as a requirement
podman doesn't work well with image-spec 1.2.0 emitting the errors above

by pinning image-spec to github.com/opencontainers/image-spec v1.0.3-0.20211202193544-a5463b7f9c84 we managed to get containerd 1.5.9 and podman 3.4.4 to work

@rhatdan
Copy link
Member

rhatdan commented Jan 14, 2022

Yes we are hoping image-spec has a new release soon so we can all get back on the same page.

@mtrmac
Copy link
Collaborator

mtrmac commented Jan 14, 2022

FWIW after containers/image#1433 , other Go modules that require image-spec 1.2.0 should no longer cause a downgrade and break the build.

@stevekuznetsov
Copy link
Contributor

@rhatdan @baude why is this issue closed? I encountered this when attempting to use the v4 bindings today. This is 10,000% still happening/

@stevekuznetsov
Copy link
Contributor

We are going to hash out the explicit things needed in #13826

@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 20, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/feature Categorizes issue or PR as related to a new feature. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

No branches or pull requests