Extended SystemContext to include VariantChoice.

[grooverdan]

Like ArchitectureChoice and OSChoice, the user has a choice of architecture variant that can be used.

[TomSweeneyRedHat]

nit, could you add a space after the %s and before the new paren? %s (variant

[TomSweeneyRedHat]

Some unhappy tests at the moment

[mtrmac]

Thanks!

• The *Choice options are overrides; first we should implement the default (choosing the right variant for the current runtime)
• This should be implemented for manifest/oci_index.go as well.
• … and copy.CheckImageDestinationForCurrentRuntime* (after #752 ).
• … so, the runtime variant detection, compatibility rules, overrides and matching logic should probably be factored out into a shared helper now that it is bigger.

[mtrmac]

The test failures are unrelated, should be eventually fixed in #744 and https://github.com/containers/skopeo/pull/760/files .

[grooverdan]

Thanks!

* The *`Choice` options are _overrides_; first we should implement the default (choosing the right variant for the current runtime)

Is including https://github.com/containerd/containerd/blob/master/platforms/cpuinfo.go#L76 exposed as the API acceptable?

* This should be implemented for `manifest/oci_index.go` as well.

Quite right. Is in the spec

* … and `copy.CheckImageDestinationForCurrentRuntime`* (after #752 ).

Thanks for the heads up, I hadn't checked PRs.

* … so, the runtime variant detection, compatibility rules, overrides and matching logic should probably be factored out into a shared helper now that it is bigger.

Like https://github.com/containerd/containerd/blob/master/platforms/compare.go ?

[mtrmac]

Thanks!

* The *`Choice` options are _overrides_; first we should implement the default (choosing the right variant for the current runtime)

Is including https://github.com/containerd/containerd/blob/master/platforms/cpuinfo.go#L76

It’s not like https://github.com/opencontainers/image-spec/blob/master/image-index.md is all that specific about the details. That’s as good a starting point as any other, I guess.

(It does vaguely feel that user-space code should be able to detect CPU variants without OS help via /proc, but I know too little about ARM, and if this code works, it can be used just fine until someone cares to it better.)

exposed as the API acceptable?

(We’ve found it difficult to keep APIs stable in c/image, so unless strictly necessary, this shared implementation etc. should be an either private within some package, or an internal subpackage (e.g. c/image/internal/…). With an internal-only API, the API design should be correct but is not that much of a concern for it to be long-term extensible and stable.)

I’m not 100% sure how the compatibility checks (that v6 images can be run on v8 systems, and that v8 images are preferred to v6 images on such systems) should be structured in the API, but, given the above, possibly.

* … so, the runtime variant detection, compatibility rules, overrides and matching logic should probably be factored out into a shared helper now that it is bigger.

Like https://github.com/containerd/containerd/blob/master/platforms/compare.go ?

Form a quick skim that seems rather over-engineered (orderedPlatformComparer / anyPlatformComparer)… given a runtime platform, we can have a hard-coded database of platform → list of supported architectures+variants in order of preference (~ a more compact version of the knowledge in platforms.Only()), and use that in the straightforward way for both the CheckImageDestinationForCurrentRuntime check and image selection from multi-arch lists.

(The os.version/os.features/features field in OCI may require significantly more complex logic, but they are underspecified enough, and the relative ordering of preference if many images match is vague enough, that I’d rather not worry generalizing any of this now. As long as the API is internal, we can always make it more complex; but if it starts complex, we will have to pay the maintenance cost unnecessarily.)

[mtrmac]

For the record, another attempt is in #786 .

[mtrmac]

This functionality was added in #854.