Skip to content
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.

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

specs-go: Respect semver 2.0.0 #1221

Closed
wants to merge 1 commit into from

Conversation

rata
Copy link
Contributor

@rata rata commented Aug 21, 2023

According to semver.org, the 2.0.0 spec, the pre-release info should be delimited by a "-" ("+" is for build info) and we already released 1.1.0, so increment the minor so this version compares to something bigger than 1.1.0.

Updates: #1220

According to semver.org, the 2.0.0 spec, the pre-release info should be
delimited by a "-" ("+" is for build info) and we already released
1.1.0, so increment the minor so this version compares to something
bigger than 1.1.0.

Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
@thaJeztah
Copy link
Member

/cc @cyphar

@thaJeztah
Copy link
Member

Note that v1.0.1-dev may not be a good choice either, as it would rank higher than v1.0.1-alpha.0 (or beta, etc).

One option could be to use v1.0.1-alpha.0, and create a tag after the (non-alpha) release is done.

In all cases, it's a bit of a "chicken-and-egg" situation, because SemVer versions are based on the content of the changes, and the content of changes is not yet known until an actual release is prepared (update Major, Minor or Patch, depending on the changes that are included).

@cyphar
Copy link
Member

cyphar commented Aug 21, 2023

  1. SemVer uses MAY in the text describing pre-release versions: "A pre-release version MAY be denoted by appending a hyphen and a series of dot separated identifiers immediately following the patch version." As per RFC 2119, not following this is not a spec violation. Contrary to the PR description, it is not a "should" (even if it were a SHOULD, it still wouldn't be a spec violation).
  2. SemVer is concerned with versions that are actually shipped. We use +dev for the HEAD of our branch, which is not shipped. As far as I'm concerned, SemVer has nothing to say about this -- we could just as easily say that the version is "DO NOT USE" for non-released versions because it is not released.
    EDIT: In other words, "pre-release" in the SemVer spec refers to things like alpha and beta releases (as shown in the examples) -- it does not refer to the HEAD of a repo branch. If it did, most projects would be breaking SemVer with every feature commit on their main branch if they aren't updating the version number each time. The fact that Go lets you use the HEAD version of a library directly is an unfortunate misfeature that Go modules attempted to fix but still support because the community depends on this now -- SemVer was not designed with Go's peculiar "versions aren't real" mantra in mind.
  3. Switching away from -dev was intentional. As mentioned in the original PR which changed this, the old behaviour was suboptimal and needed to be changed. For one, -dev ranks higher than -alpha and -beta. It also has the frustrating behaviour of making emergency updates cause issues with spec validation tools because the version (according to SemVer) is different -- there is no ideal solution for the spec validation problem, but -dev is the worst solution. The only alternative solution that seems reasonable is to switch to the Go model (1.1.1-0.gitfoo+unreleased) but this has other downsides. If you feel that +dev is a problem, please help us come up with a solution other than the old one (which we moved away from for a reason).

Copy link
Member

@cyphar cyphar left a comment

Choose a reason for hiding this comment

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

nack

giuseppe
giuseppe previously approved these changes Aug 21, 2023
Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

LGTM

@giuseppe giuseppe dismissed their stale review August 21, 2023 15:39

given the other review, I am taking the LGTM back

@rata
Copy link
Contributor Author

rata commented Aug 22, 2023

@cyphar

  1. And why you mention that? It is completely unrelated to what I say. Am I missing something? I just say we treat "1.0.2-dev" as older than "1.0.2", but on semver it isn't. I'm not saying something break without a pre-release info.
  2. But as I said, this is exposed to users: runc features in 1.1. exposed version 1.0.2-dev.

@thaJeztah thanks for the links, I've missed that. And I had a typo in my example when comparing "-dev" versions with "alpha.1", so I thought the comparisson worked fine, when it didn't :-D

@rata
Copy link
Contributor Author

rata commented Aug 22, 2023

@cyphar
3. Why not use 1.1.1-0.unreleased then? This compares to something bigger than 1.1.0, so users can see that is newer and it is smaller than all the pre-releases we might make (1.1.1-alpha, -alpha.1, beta, or even 1.2.0, all works fine)

I don't think using an unreleased version creates a new problem for runtime-tools. It only supports to validate the current spec-version vendored, so if the exact same version is vendored, it will work. If it isn't, it won't (same as with the curren "-dev" we have today).

rata added a commit to kinvolk/crun that referenced this pull request Aug 24, 2023
This PR upstream added the spec for the mountExtensions feature field:
	opencontainers/runtime-spec#1219

This commit just implements that and updates the OCI max version
implemented to the one currently used in the spec (an unreleased
version).

It is not clear if in the future, the version of unreleased specs will
be changed to something else:
	opencontainers/runtime-spec#1221

But this is what is currently accepted.

Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
@rata
Copy link
Contributor Author

rata commented Nov 2, 2023

Now that we merged the way to expose the features, this is not so important to me. If indeed exposing in runc different specs but whose versions compare to equal is problematic (I wouldn't be surprised if it is), we can revisit this decision.

@rata rata closed this Nov 2, 2023
@rata rata deleted the rata/fix-versioning branch November 2, 2023 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants