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

Merge SemVer and specVersion #377

Closed
wants to merge 1 commit into from

Conversation

armintaenzertng
Copy link
Contributor

@armintaenzertng armintaenzertng commented Jun 7, 2023

this is part of #368

Note: This moves the heading "Format" to Properties files, so the spec-parser needs to be adapted.

Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
@kestewart kestewart requested review from zvr and goneall June 14, 2023 03:09
@kestewart kestewart added this to the 3.0-rc2 milestone Jun 14, 2023
Copy link
Member

@goneall goneall left a comment

Choose a reason for hiding this comment

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

Agree with the approach - @zvr should we wait for the spec parser to be updated before merging?

@armintaenzertng
Copy link
Contributor Author

I think updating the parser before merging would be better, else we get a continuously failing pipeline again that everybody simply ignores.
I will look into this.

@armintaenzertng
Copy link
Contributor Author

@zvr, @goneall: The spec-parser PR is ready for review here: spdx/spec-parser#57

@davaya
Copy link
Contributor

davaya commented Jun 15, 2023

The pattern in specVersion.md conflicts with the SemVer specification:

  1. A normal version number MUST take the form X.Y.Z where X, Y, and Z are non-negative integers, and MUST NOT contain leading zeroes. X is the major version, Y is the minor version, and Z is the patch version. Each element MUST increase numerically. For instance: 1.9.0 -> 1.10.0 -> 1.11.0.

Issue 1: v3.0.0 that appears in our examples is not valid according to SemVer. Do we want to allow alpha characters in SPDX versions or just non-negative integers?

Issue 2: SPDX is a standard, not a software product. Do we want to force all specVersions to have a patch version, or should 2.3 and 3.0 be valid SPDX versions?

Issue 3: Since SemVer is used by more than just SPDX, should SemVer be left as a reusable Core type that could be included in profile-defined Elements?

@armintaenzertng
Copy link
Contributor Author

@davaya: Your issues 1 and 2 should be separate issues from this PR as I did not change anything regarding the formatting of specVersion.
Regarding your issue 3: When this PR is merged, there is currently nothing in the spec that references the SemVer type. I don't think we should keep it around simply for the possibility that it might be used later. We can always reintroduce it at that point.

@davaya
Copy link
Contributor

davaya commented Jun 15, 2023

Gary agrees with getting rid of the SemVer type, and I don't have a strong objection. But there should be a good rationale for getting rid of something that already exists in Core today and might be used and re-introduced in the future. I don't think there is a great benefit to making this change.

IF we got rid of it, and
IF some future element used an anonymous semver pattern instead of a SemVer type, and
IF we decided to make a change to the pattern based on issues 1 and 2,
THEN we'd need to make the change in multiple places instead of one place.

That's a speculative scenario, but I always like to promote re-use by default if it causes no harm. What is the harm in keeping it?

@zvr
Copy link
Member

zvr commented Jun 15, 2023

The pattern in specVersion.md conflicts with the SemVer specification:

Can you explain, @davaya ? If this is the regex pattern, I took it from the official semver.org site.

Other comments:

  • v3.0.0 is not valid, the initial v must be deleted
  • we have decided we will use semantic versioning, as defined in semver.org

This PR gets rid of the SemVer class, and changes the property to be a string with a value-matching restriction.
The alternative would have been to have a class with an extra property for this, since you can't really subclass data types like xsd:string.

@davaya
Copy link
Contributor

davaya commented Jun 22, 2023

Can you explain, @davaya ? If this is the regex pattern, I took it from the official semver.org site.

The model file is currently:

## Metadata
- name: SemVer
- SubclassOf: xsd:string

## Properties

## Format
- pattern: ^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-((?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\+([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?$

The format pattern allows things like RCs and alpha/betas that are useful for software releases, but it is more permissive than SemVer item 2 that uses only integers. The latter seems more appropriate for documents - do we ever want to have elements with specVersion = "1.0.0-alpha+001" or "1.0.0+20130313144700" or "1.0.0-beta+exp.sha.5114f85"?

This PR gets rid of the SemVer class, and changes the property to be a string with a value-matching restriction. The alternative would have been to have a class with an extra property for this, since you can't really subclass data types like xsd:string.

XSD doesn't use the word "class", it has its normal English meaning: a category of things as opposed to instances of that category. Putting restrictions like length or range or pattern on fundamental classes like string and integer is "subclassing" them. For example, "Age" is a SubclassOf: "xs:integer" with range restriction of 0-120 years: https://www.w3schools.com/xml/schema_facets.asp.

Being able to re-use restricted fundamental types like SemVer or Age is the reason for defining the restrictions in Class model files. Getting rid of re-usable definitions is a step backwards.

@zvr
Copy link
Member

zvr commented Jun 23, 2023

To the first point, yes, we definitely will be using 3.1.0-rc1 and such things. That's why we went to semver instead of staying with X.Y as SPDXv2.

To the second point, the link you provided shows exactly what I've been saying above; you can add a restriction to the value of the property age. You cannot define something ("class", "type", ...) named Age and have properties being of this something.

This is what this PR is doing. Since we cannot have SemVer, we add the restrictions to the property specVersion.

@davaya
Copy link
Contributor

davaya commented Jun 23, 2023

Semantics means "meaning", and the logical model is intended to capture meaning to enable reasoning about data. The semantics of SemVer is that it has major, minor, and patch integers, as well as pre-release and build metadata components.

A reasoner should be able to ask for all spdx v3 (major version) elements or all pre-release elements and software should be able to find them. To enable that, SemVer needs to be a Class with five properties, not a string. Serialization turns those five properties into a single string for most applications to use, but semantic applications parse that string back into named components that can be processed in a graph.

SPDX-License-Identifier: Community-Spec-1.0

# SemVer

## Summary
A version designator that follows the SemVer 2.0.0 specification.

## Description
A semantic version has semantics defined by the specification of [Semantic Versioning 2.0.0](https://semver.org/).

## Metadata
- name: SemVer
- Instantiability: Concrete

## Properties
- major
  - type: xs:integer
  - minCount: 1
  - maxCount: 1
  - minValue: 0
- minor
  - type: xsd:integer
  - minCount: 1
  - maxCount: 1
  - minValue: 0
- patch
  - type: xsd:integer
  - minCount: 1
  - maxCount: 1
  - minValue: 1
- preRelease
  - type: xsd:string
  - minCount: 0
  - maxCount: 1
  - pattern: ^-((?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*)$
- buildInfo
  - type: xsd:string
  - minCount: 0
  - maxCount: 1
  - pattern: ^\+([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*)$
 

@davaya
Copy link
Contributor

davaya commented Jun 24, 2023

You cannot define something ("class", "type", ...) named Age and have properties being of this something.

How so? The example in https://www.w3.org/TR/xmlschema11-1/#Complex_Type_Definitions shows a PurchaseOrderType containing several properties with name and type attributes.

A CreationInfo XML instance (shortened to two properties for illustration), and in XML format instead of JSON since we're defining types using XSD rather than JSON Schema, would be:

<CreationInfo>
  <specVersion>3.0.0</specVersion>
  <created>2022-12-01T00:00:00</created>
</CreationInfo>

with the corresponding Class definition following the PurchaseOrderType example:

<?xml version="1.0" encoding="UTF-8"?>
<xs:schema xmlns:xs="http://www.w3.org/2001/XMLSchema">
  <xs:simpleType name="SemVer">
    <xs:restriction base="xs:string">
      <xs:pattern value="(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)" />
    </xs:restriction>
  </xs:simpleType>
  <xs:element name="CreationInfo">
    <xs:complexType>
      <xs:sequence>
        <xs:element name="specVersion" type="SemVer" />
        <xs:element name="created" type="xs:string" />
      </xs:sequence>
    </xs:complexType>
  </xs:element>
</xs:schema>

The SemVer type (with the pattern shortened to three digits) validates 3.0.0 but correctly rejects v3.0.0, demonstrating that the SemVer type works fine whether you call it a "subclass" of its base xs:string or not. The same SemVer can of course be defined in JSON Schema in a #definitions list like all other types, and used as a property type like all other types.

The above suggestion to make SemVer a structure can be accepted or rejected on its own merits, but the ability to define a SemVer Class to specify property types doesn't depend on what the type looks like (string or structure).

The question is whether SemVer and DateTime types should be defined so that they can be used in more than one place. I believe they are generally useful and thus should be defined in Class files. The SubclassOf metadata item is blank or explicitly "none" for all datatypes except xsd:string; if it is treated consistently with the others this perceived problem disappears.

@armintaenzertng
Copy link
Contributor Author

This is obsoleted by #424.

@armintaenzertng armintaenzertng deleted the semVer-fix branch July 26, 2023 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants