I think we should rethink some of this; Parse should really do just that parse what's in the string format, and return the results.

We could consider a functional argument to provide defaults, but it's problematic that we start to fill in missing information; doing so means that we make assumptions based on the local platform , which can be problematic. See the discussion below for more context;
moby/buildkit#4315 (comment)

We should also formalize the string format; "/variant" (optional) and "/os-version" (also optional) can be ambiguous, which means we won't be able to tell for sure what's what.

/cc @dmcgowan @tonistiigi

Also related; I once started a draft where I looked at how we currently handle some of this (but never found time to finish); here's a gist of some of the notes I drafted; https://gist.github.com/thaJeztah/a970f7b93f5f0a68b36b5dbb755c320d

This looks like something done in the rest of the cases in the function now. Is this something that needs to be solved for this PR or more wholistically?

This empty variant double quote idea is really weird and I don't think we can expect users to write something like this. (I don't understand where the need for double quote specifically is coming from as well).

My recommendation would be to strictly define the cases where osversion is allowed and from there deduce the packing rules between variant and osversion so either or both can be present in practical cases. We already have many known values for arch and variants.

I also think we should define normalization rules like they exist for variants in amd64, arm and arm64 so that empty osversion is equal to a specific value defined for windows architecture and setting that value would normalize it to empty osversion in string form.

Right now, empty osversion is a valid value for os==windows, and containers exist with that value in their config JSON. They're the ones most-similar to non-Windows containers, as they contain no OS components inside the container itself, and we need to be able to match/exclude such containers. So the string-form empty string is most naturally going to be the empty string when parsed.

I'm not clear why windows/amd64//10.0.20348 wasn't the chosen direction, actually. That's what I would have started with...

as they contain no OS components inside the container itself, and we need to be able to match/exclude such containers. So the string-form empty string is most naturally going to be the empty string when parsed.

What does that match rule mean in that case? If we would chose osversion that gets normalized to empty, I would assume it would be the one that is most common and compatible with most systems (as v1 is for amd64, or v8 for arm64, for example, most people do not understand that it is even there).

It's actually the other way 'round in this case. The only way a Windows container image with no OS components can be executed is in Host Process mode, aka most-privileged mode. It can also be used to signal a container image being used to distribute Windows binaries but that is not expected to be executed itself, e.g., the containerd system for distributing runtime shims in registries.

Conversely, when you're specifying a platform you want to run, then "" is the most-compatible value, as Host Process mode can run any container, irrespective of OSVersion, it just ignores the OS Components in the container AFAIK.

However, it makes a poor default because nerdctl run --platform windows/amd64 is very unlikely to actually mean Host Process mode, unless explicitly set (which nerdctl maybe lacks CLI for...) and instead should mean "Process isolation" so in that case the best default parse is "Current host OSVersion". I think this is where the current behaviour of platforms.Parse comes from, but it makes other use-cases for platforms.Parse problematic.

The Windows OS ABIs are not fully forward/backward compatible, so there isn't really a good "Runs everywhere" OS Version value like Architecture Variants have for Process Isolation (the most common isolation case); so any most-common value chosen is only correct for a limited period of time, and then its usage drops off over time, and is eventually end-of-lifed. With Hyper-V Isolation, the most-compatible variant is Windows Server 2016, but that's end-of-lifed and well out-of-support, so is clearly a poor choice.

In terms of Windows platform matching, the matcher needs to know what host environment (version and isolation mode) the container will be run in, so it can set the constraints on OSVersion correctly. This is easy when you're actually running the container, as that knowledge is there. So I think the matcher is a better place to do things like defaulting OSVersion to the host. (That's moby/buildkit#4315 (comment) again. Maybe we should fix that behaviour first... Or at least agree to fix it in some way, and then see what (if anything) that shakes out for this use-case.)

However, things like nerdctl pull --platform windows/amd64 some/image:index are a little tricker, as you would need to guess if the user meant "Pull for run in Process Isolation" in which case you need to insert the host OS Version, or "Pull to run in Host Process, or to simply extract some files" in which case you must not insert the host OS Version, or you'll pull a different image than the eventual nerdctl run or nerdctl copy will use. (This is a place where the defaults added by platform.Parse will cause issues, as the caller doesn't know if the default was inserted or explicitly specified, losing the user-intent.)

Maybe this makes the windows(osversion) idea more palatable? windows() would be explicitly "no OS components", and windows could mean "Current host ABI".

Of course, non-Windows hosts aren't going to be able to produce a rational value for "Current host ABI" but in such a case, they can't run the container image anyway so that closes off a bunch of places we actually care about osversion, and for other cases, they probably need to be explicit anyway. As a default in this case, the Host Process value (whatever it is, currently osversion=="" in live images) makes sense, as matching against that has the widest set of matches.

I don't think would be correct to not fill in the OSVersion altogether for windows as that would mean HPC. I think for the time being we can let that behavior be and extend platforms.Parser() and platforms.Format() to account for OSVersion if one was specified. We could maybe use the format of OS/architecture/variant/OSVersion ? Once the changes for image pull per runtime class goes in, we would need to support option of specifying the OSVersion for windows.
Currently, the default platform matcher for windows does fill in the OS version of the host

IMO, we should be fixing on a format that platforms.Format() takes and parses. It should honor OSVersion if one was specified and not fill in the OSVersion by default. variant for windows OS and amd64 architecture is "" so the format could be windows/amd64// . Thoughts?

cc @jsturtevant @marosset @aravindhp @kevpar

However, it makes a poor default because nerdctl run --platform windows/amd64 is very unlikely to actually mean Host Process mode, unless explicitly set (which nerdctl maybe lacks CLI for...) and instead should mean "Process isolation" so in that case the best default parse is "Current host OSVersion". I think this is where the current behaviour of platforms.Parse comes from, but it makes other use-cases for platforms.Parse problematic.

But --platform windows/amd64 does not necessarily need to be the same as no --platform on wcow machine. If no platform is set then DefaultSpec reads current host and gets the current os-version and most precise match for the current system. Otoh --platform windows/amd64 should resolve same bytes for all contexts. If it is building image that is not meant to just run on the current system, or cross-compile in linux, or building multiple variants with different OSVersion and choosing the correct base image each case. All these cases should generate the same result if --platform was defined, independently of the builder host configuration.

I agree that when building an image, --platform windows/amd64 explicitly should give the same result in all contexts, but I don't believe we can choose a one-true-forerver-correct default unless it's either Host-Process Container/No OS components, or is going to change as "The current Windows ABI" changes over Windows Server releases (or rather, with BuildKit releases that include that updated knowledge). So "--platform windows/amd64 will give the same result for all contexts in a given BuildKit release" is the strongest I believe we can achieve, if we don't go with Host-Process Container/No OS components for this case.

In the quoted text, I was talking about nerdctl run where the user intent is clearly "Execute the container image", so a working configuration should be the outcome when possible. Implying --privileged when a reasonable-looking --platform windows/amd64 without explicit osversion should, IMO, be less-preferenced than implying an osversion that the host configuration can run.

So in the end, I think filling in an unspeficied osversion is a BuildKit thing (or other platforms.Parse caller) as only that code knows what the user's intent was for that string, and containerd's platforms.Parse should not be trying to make such guesses or carry such knowledge in/out. Maybe in the case of BuildKit --platform windows/amd64 should mean mean "No OS components" and proceed happily, but it's all-but-nonsensical to mean that in nerdctl run without --privileged.

All that said, using osversion=="" as both "unspecified" and "No OS Components included" is problematic as the caller of platforms.Parse can't distinguish them, which in this formulation, is important.

Maybe we should add a "NONE" value for osversion, and then the PlatformMatcher can know that this can match images with "" for osversion (which is not coming via platforms.Parse and hence does not suffer the ambiguity) since they are already in the wild. It becomes the caller's job to hide that NONE before passing the Platform object on, which is ugly. The user might need to call BuildKit with --platform windows/amd64//NONE or --platform windows/amd64//, which is ugly, when they explicitly want to build Host Process Only or unrunnable images. So this paragraph needs much more refining and bikeshedding before it's actionable. (If Go strings could be nil, or the optional fields in Platform had been pointers, there'd be easy options.)

Also, if you specify an osversion in --platform for the builder, but use a base image with a different version, that should fail, right? Containerd downwards doesn't care about the osversion for executing a fetched image, as the image's metadata controls all that. Similarly, FROM scratch only makes sense for Host Process Only or unrunnable container images.

Does non-Windows have the equivalent of unrunnable images? Like, if you have an image that contains only data, no executables, does it need to have an os and arch value, or can we use "" there too?

Late edit: I wrote the above before I saw that the main thread has shaken out towards windows(version=) syntax. I don't think it changes any of the above, but I haven't gone back and confirmed that or rewritten the examples.