opencontainers / runtime-spec

OCI Runtime Specification
http://www.opencontainers.org
Apache License 2.0
3.13k stars 535 forks source link

dev versions don't respect semver #1220

Closed rata closed 8 months ago

rata commented 10 months ago

When we switch to dev after a release, we are not respecting semver (as defined in semver.org version 2.0.0). The issue is that we just change the patchDev to "-dev", without changing the minor or anything else.

After release x.y.0, we almost always switch that field to "-dev". The issue is that according the spec, x.y.0-dev is a lower version than x.y.0. Therefore, we should increase some other component while switching to "-dev". Like x.y.1-dev.

This is not a big issue generally, as this dev versions are not included in releases, but it become an issue since we expose this in "runc features". This is incovenient, as using modules like https://pkg.go.dev/golang.org/x/mod/semver will not give the desired result when we compare versions (with our versioning, "1.0.2-dev" is greater than "1.0.2", but not with that module that correctly implements the spec).

Looking at the git history, we've almost always done that, the only exception is recent: https://github.com/opencontainers/runtime-spec/commit/3f552ce17e77ef28a21764f7c499b555517afa43 (what is currently on main). The + sign should be used to indicate a build, which is incorrect here.

In fact, if we look at the go.mod in runc release-1.1, it shows this that is in sync with what I propose in the next paragraph:

        github.com/opencontainers/runtime-spec v1.0.3-0.20210326190908-1c3f411f0417

I propose the following:

  1. Switch main from 1.1.0+dev to 1.1.1-dev (respects semver and is a greater version than 1.1.0)
  2. The feature in runc 1.1 is experimental, so just in the next 1.1 patch release, change the version reported from "1.0.2-dev" to "1.0.3-dev" (hardcode the string in the release-1.1 branch, that is simple and should do it).
  3. Unsure how to add a test case that will enforce this, but at least add a big comment in the specs-go/version.go file about it.

Note there is nothing to do for crun (it implemented it in 1.8.6, that uses an rc runtime spec release all good in 1.8.7 and main)


cc @giuseppe @AkihiroSuda

cyphar commented 10 months ago

The reasoning behind this change is in #1198. Changing back to -dev is not acceptable. Please read through the discussion there and if you feel there is a better solution, please propose that instead.

I think the model used by Go (which is actually not what you are proposing -- using -0 as the pre-release tag is a deliberate choice made by Go) has some upsides, but it has the following downsides (I talk about these at length in #1198):

  1. We create fake releases that may not exist. SemVer doesn't care about this, but users might (if they see 1.0.3-0.foobar in their configs, they would understandably assume that they are using version 1.0.3 of the spec -- which they are not). If we ever decide to have release branches, this will be an even bigger issue because the in-development "version" string for the main branch will conflict with the versioning for the older release branch unless we are very careful and have 1.2.0-0 as the main branch (which diverges from how Go handles this).
  2. If we want to extend the spec validation tools to be able to handle unknown versions more reasonably, being able to mark the in-development version of the spec as being "this version plus some patches" is far easier for these tools to detect (and thus more preferable) than having a fake version which the tool will need to make guesses to figure out whether it is a version it can audit.

In addition, ideally nobody should be using the +dev "versions" of the spec, because they are explicitly unreleased. Unfortunately, we have had to do this in the past and so having a somewhat reasonable "version" string is necessary -- but it should be unusual for a project to use unreleased OCI specifications.

In addition (as I mentioned in #1221), you keep using the word "should" but the specification explicitly uses the word MAY -- meaning that us deviating from these recommendations is perfectly acceptable and requires no justification (with SHOULD we need to have some justification -- which I think we do -- but it would still be allowed). And, SemVer doesn't cover unreleased in-development "versions" of the spec. Go's behaviour exists because they needed to figure out a way to retro-fit their previous "versions shouldn't exist" mantra onto the Go module system (where everything is SemVer). It's a very clever hack, but it is still a hack.

rata commented 10 months ago

@cyphar

Unfortunately, we have had to do this in the past and so having a somewhat reasonable "version" string is necessary -- but it should be unusual for a project to use unreleased OCI specifications.

We do this in runc all the time. I don't see why you think it should be unusual, it's the opposite: we even implement some spec changes on runc to validate the changes we propose to the spec. To do that, we use an unreleased version of the spec.

And unless we want to start releasing new versions of the spec every few months, I don't see how this can change (and I'm not sure releasing every few months will be enough to not need this). And I don't think adding more bureaucracy, like requiring a spec release to use it in runc, is going to help in any way.

In addition (as I mentioned in #1221), you keep using the word "should" but the specification explicitly uses the word MAY

If I don't use it in capital letters, don't take it as the RFC meaning.

cyphar commented 10 months ago

If I don't use it in capital letters, don't take it as the RFC meaning.

For what it's worth, I think the usage of "should" here is incorrect in the regular meaning as well. Titling the issue "dev versions don't respect semver" implies that this is a spec violation (if it's not a violation, there is nothing to "respect") and you used the phrase:

According to semver.org, the 2.0.0 spec, the pre-release info should be delimited by a "-" ("+" is for build info) [emphasis added]

Even if it's not in capitals, the implication is that the spec is saying we should do something, when that is not the case (neither in the RFC sense of the word, nor in the regular sense of the word).

rata commented 10 months ago

@cyphar

According to semver.org, the 2.0.0 spec, the pre-release info should be delimited by a "-" ("+" is for build info) [emphasis added]

Even if it's not in capitals, the implication is that the spec is saying we should do something, when that is not the case (neither in the RFC sense of the word, nor in the regular sense of the word).

What I meant with that is that the "pre-release" info is supposed to be used in the "-pre-release" part of the format, the "+build" thing is for the build information. We are not respecting that and I think we should. Simply because things that are different, under semver, evaluate to equal.

Now that the mountExtensions PR is merged (here, runc and crun), I can use that to see idmap mounts support and I don't have to special-case the broken runtime-spec versions (regarding semver) in containerd and this issue is less important to me in practical terms.

cyphar commented 10 months ago

My point was that "pre-release" in SemVer explicitly refers to alpha and beta releases, it doesn't refer to unreleased HEAD commits (what you are referring to as "pre-releases").

rata commented 8 months ago

Closing for the same reason as stated here: https://github.com/opencontainers/runtime-spec/pull/1221#issuecomment-1791242162