Update keyctl for other arches #675
Conversation
Use definitions from golang.org/x/sys/unix to get the keyctl package building on other Linux architectures. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
|
@QiWang19 PTAL. I'm not sure why we have a copy of github.com/jsipprell/keyctl instead of vendoring it as a dependency. |
|
WRT having a copy, see the earlier conversation in #619 ; things did not out quite as painless as expected. |
|
Ah, okay. I missed that detail while skimming it earlier. Thanks! |
| func (k *Key) ID() int32 { | ||
| return int32(k.id) | ||
| func (k *Key) ID() int { | ||
| return int(k.id) |
There was a problem hiding this comment.
Strictly speaking, this is an API break. Do we need to change this? int would certainly be more idiomatic Go; still, the kernel key_serial_t is an int32_t, so this does not seem to hurt anything.
Same for ID.ID / Keyring.ID.
There was a problem hiding this comment.
Ah, you're right. The APIs in golang.org/x/sys/unix use int for key types, so I changed the package to match without considering that.
| } | ||
| return int32(r1), nil | ||
| func (id keyID) ID() int { | ||
| return int(id) |
There was a problem hiding this comment.
Non-blocking: AFAICS this method can be safely dropped; all keyID values are private, so no external caller can call this (and internal ones just use int(keyID) AFAICS.)
There was a problem hiding this comment.
I think you're right - the only consumer I'm aware of is elsewhere in the library. Would you like a follow-up PR?
| @@ -3,11 +3,14 @@ | |||
| // license that can be found in the LICENSE file. | |||
|
|
|||
| // +build linux | |||
| // +build 386 amd64 | |||
|
|
|||
| // Package keyctl is a Go interface to linux kernel keyrings (keyctl interface) | |||
There was a problem hiding this comment.
Non-blocking, RFC: Maybe add a note here that the package is deprecated, and most callers should call x/sys/unix directly.
|
LGTM with @mtrmac comments. |
|
Addresses: containers/buildah#1762 |
|
@mtrmac 's comments were not yet addressed. |
|
Once the dust settles I'm assuming we're respinning 3.0.0 or 3.0.1? |
|
3.0.1 ... 3.0.0 is already out |
|
Follow-up: #676. PTAL. |
We do have cross-OS compilation in there. Cross-architecture is not trivial: $ GOOS=linux GOARCH=arm64 go build -tags containers_image_openpgp ./...
vendor/github.com/containers/storage/drivers/overlay/overlay.go:21:2: build constraints exclude all Go files in $GOPATH/src/github.com/containers/image/vendor/github.com/containers/storage/drivers/quota
# exclude_graphdriver_overlay is not really representative, but let’s try:
macOS $ GOOS=linux GOARCH=arm64 go build -tags 'containers_image_openpgp exclude_graphdriver_overlay' ./...
# github.com/containers/image/vendor/github.com/containers/storage/pkg/loopback
vendor/github.com/containers/storage/pkg/loopback/attach_loopback.go:21:36: undefined: LoNameSize
# and many other failuresI guess the macOS failures could go away on a Linux box, and c/image CI maybe does not need to worry about representative c/storage builds. |
Use definitions from
golang.org/x/sys/unixto get thekeyctlpackage building on other Linux architectures.