Skip to content

platforms: define selectors for platforms #1403

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.

Sign up for GitHub

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

Merged
dmcgowan merged 3 commits into from
Sep 13, 2017
Merged

platforms: define selectors for platforms #1403

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fb06883
platforms: define selectors for platforms
stevvooe Aug 19, 2017
94f6be5
platforms: implement matcher support
stevvooe Sep 9, 2017
775f7ce
platforms: update format for platform specifier
stevvooe Sep 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions differ/differ.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ func (s *walkingDiff) Apply(ctx context.Context, desc ocispec.Descriptor, mounts
if strings.HasSuffix(desc.MediaType, ".tar.gzip") || strings.HasSuffix(desc.MediaType, ".tar+gzip") {
isCompressed = true
} else if !strings.HasSuffix(desc.MediaType, ".tar") {
return emptyDesc, errors.Wrapf(errdefs.ErrNotSupported, "unsupported diff media type: %v", desc.MediaType)
return emptyDesc, errors.Wrapf(errdefs.ErrNotImplemented, "unsupported diff media type: %v", desc.MediaType)
}
}

Expand Down Expand Up @@ -128,7 +128,7 @@ func (s *walkingDiff) DiffMounts(ctx context.Context, lower, upper []mount.Mount
media = ocispec.MediaTypeImageLayerGzip
isCompressed = true
default:
return emptyDesc, errors.Wrapf(errdefs.ErrNotSupported, "unsupported diff media type: %v", media)
return emptyDesc, errors.Wrapf(errdefs.ErrNotImplemented, "unsupported diff media type: %v", media)
}
aDir, err := ioutil.TempDir("", "left-")
if err != nil {
Expand Down
6 changes: 3 additions & 3 deletions errdefs/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ var (
ErrAlreadyExists = errors.New("already exists")
ErrFailedPrecondition = errors.New("failed precondition")
ErrUnavailable = errors.New("unavailable")
ErrNotSupported = errors.New("not supported") // represents not supported and unimplemented
ErrNotImplemented = errors.New("not implemented") // represents not supported and unimplemented
)

func IsInvalidArgument(err error) bool {
Expand Down Expand Up @@ -54,6 +54,6 @@ func IsUnavailable(err error) bool {
return errors.Cause(err) == ErrUnavailable
}

func IsNotSupported(err error) bool {
return errors.Cause(err) == ErrNotSupported
func IsNotImplemented(err error) bool {
return errors.Cause(err) == ErrNotImplemented
}
4 changes: 2 additions & 2 deletions errdefs/grpc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ func ToGRPC(err error) error {
return grpc.Errorf(codes.FailedPrecondition, err.Error())
case IsUnavailable(err):
return grpc.Errorf(codes.Unavailable, err.Error())
case IsNotSupported(err):
case IsNotImplemented(err):
return grpc.Errorf(codes.Unimplemented, err.Error())
}

Expand Down Expand Up @@ -72,7 +72,7 @@ func FromGRPC(err error) error {
case codes.FailedPrecondition:
cls = ErrFailedPrecondition
case codes.Unimplemented:
cls = ErrNotSupported
cls = ErrNotImplemented
default:
cls = ErrUnknown
}
Expand Down
77 changes: 77 additions & 0 deletions platforms/database.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
package platforms

import (
"runtime"
"strings"
)

// These function are generated from from https://golang.org/src/go/build/syslist.go.
//
// We use switch statements because they are slightly faster than map lookups
// and use a little less memory.

// isKnownOS returns true if we know about the operating system.
//
// The OS value should be normalized before calling this function.
func isKnownOS(os string) bool {
switch os {
case "android", "darwin", "dragonfly", "freebsd", "linux", "nacl", "netbsd", "openbsd", "plan9", "solaris", "windows", "zos":
return true
}
return false
}

// isKnownArch returns true if we know about the architecture.
//
// The arch value should be normalized before being passed to this function.
func isKnownArch(arch string) bool {
switch arch {
case "386", "amd64", "amd64p32", "arm", "armbe", "arm64", "arm64be", "ppc64", "ppc64le", "mips", "mipsle", "mips64", "mips64le", "mips64p32", "mips64p32le", "ppc", "s390", "s390x", "sparc", "sparc64":
return true
}
return false
}

func normalizeOS(os string) string {
if os == "" {
return runtime.GOOS
}
os = strings.ToLower(os)

switch os {
case "macos":
os = "darwin"
}
return os
}

// normalizeArch normalizes the architecture.
func normalizeArch(arch, variant string) (string, string) {
arch, variant = strings.ToLower(arch), strings.ToLower(variant)
switch arch {
case "i386":
arch = "386"
variant = ""
case "x86_64", "x86-64":
arch = "amd64"
variant = ""
Copy link
Contributor

@rn rn Aug 30, 2017

Choose a reason for hiding this comment

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

I'm not sure one wants to do this matching above of commonly used but not part of the OCI spec names for architectures.

I would suggest to simply just use the names from the spec and that's it. Otherwise you'd have to do the same of OSes too. darwin is now officially called macOS, for example. If you translate x86-64 to amd64 why not translate macOS to darwin?

Copy link
Member Author

@stevvooe stevvooe Aug 30, 2017

Choose a reason for hiding this comment

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

We can add a translation for macOS.

Copy link
Member Author

@stevvooe stevvooe Aug 30, 2017

Choose a reason for hiding this comment

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

I'm not sure one wants to do this matching above of commonly used but not part of the OCI spec names for architectures.

I'd prefer a strict approach, but I think we can make everyone happy here because the sets (os/arch) are disjoint. To keep this nice property across projects, the normalization needs to be centralized.

Do you see an issue with this in the future?

Copy link
Contributor

@rn rn Aug 31, 2017

Choose a reason for hiding this comment

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

I have to say that at time I find the mismatch between uname -m and Go architectures (and therefor OCI architectures) quite annoying, so I can see an argument for tools to accept both (and then internally translate to the canonical GOARCH/GOOS format). But I can also see an argument for keeping it simple and just use OCI names and leave it to the user to translate.

For now I don't see any issues in the future (I left my crystal ball at home) so if the general desire is to allow commonly used alternative arch/OS names to be used, centralising it in one place like this PR tries certainly a good idea.

case "aarch64":
arch = "arm64"
variant = "" // v8 is implied
Copy link
Member

@AkihiroSuda AkihiroSuda Aug 20, 2017

Choose a reason for hiding this comment

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

if variant == "v8" { variant = ""}? (for better support for future non-v8 variants)

Copy link
Member Author

@stevvooe stevvooe Aug 21, 2017

Choose a reason for hiding this comment

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

arm64 always defaults to v8. If there are future variants, they will be qualified as such.

case "armhf":
arch = "arm"
variant = ""
Copy link
Contributor

@ijc ijc Aug 31, 2017

Choose a reason for hiding this comment

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

We normalise (below) linux/arm into linux/arm/v7 but here we are normalizing linux/armhf into just linux/arm, which seems a bit non-normalysed to me.

IOW if the result of normalizeArch was actually normative (as I think it should be) then the assertion in the following should always be true:

foo := normalizeArch(input)
assert(foo == normalizeArch(foo))

but that doesn't appear to be the case when input == "linux/armhf"

Copy link
Contributor

@ijc ijc Aug 31, 2017

Choose a reason for hiding this comment

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

TL;DR: I think you can ignore my previous comment.

I think I misread the code handling the "arm" case, that switch doesn't actually have the default case I thought it did so normalizeArch("arm") == "arm" as required.

(I'm also aware I was taking some liberties with the actual arguments, there should be split("/", foo) stuff sprinkled around in my examples to make them closer to actually working examples)

Copy link
Member Author

@stevvooe stevvooe Aug 31, 2017

Choose a reason for hiding this comment

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

Just added a test case to ensure the round trip.

case "armel":
arch = "arm"
variant = "v6"
case "arm":
switch variant {
case "v7", "7":
variant = "v7"
Copy link

@johnstep johnstep Sep 7, 2017

Choose a reason for hiding this comment

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

Why include the v7 case, since it doesn't change? Is it just for clarity, to assign variant in all non-default cases?

Copy link
Member Author

@stevvooe stevvooe Sep 7, 2017

Choose a reason for hiding this comment

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

Mostly to prevent it going down other control flow paths.

case "5", "6", "8":
variant = "v" + variant
}
}

return arch, variant
}
16 changes: 16 additions & 0 deletions platforms/defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package platforms

import (
"runtime"

specs "github.com/opencontainers/image-spec/specs-go/v1"
)

// Default returns the current platform's default platform specification.
func Default() specs.Platform {
return specs.Platform{
OS: runtime.GOOS,
Architecture: runtime.GOARCH,
// TODO(stevvooe): Need to resolve GOARM for arm hosts.
}
}
20 changes: 20 additions & 0 deletions platforms/defaults_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package platforms

import (
"reflect"
"runtime"
"testing"

specs "github.com/opencontainers/image-spec/specs-go/v1"
)

func TestDefault(t *testing.T) {
expected := specs.Platform{
OS: runtime.GOOS,
Architecture: runtime.GOARCH,
}
p := Default()
if !reflect.DeepEqual(p, expected) {
t.Fatalf("default platform not as expected: %#v != %#v", p, expected)
}
}
Loading