VersionPrefix and VersionSuffix are misnamed

[rainersigwald]

From @adamralph on April 7, 2017 6:44

Prefix:

a word, letter, or number placed before another.

The value of this property never prefixes the version. That would imply that the version is another string of characters, and the value of this property precedes it. This never happens.

Until I read the documentation, I was surprised to see values of VersionPrefix of the form "1.0.0", since "1.0.0" is not a prefix to the version. It is the version.

The same argument could be applied to VersionSuffix, depending on the interpretation of "version". If "1.0.0" is the "version" in "1.0.0-beta1" then it's true to say that the "version suffix" is "beta1", but if "1.0.0-beta1" is the "version", then VersionSuffix is similarly misnamed.

---

Update:

Based on the comments below, it seems that the general opinion is that "1.0.0-beta1" is the "version", implying that VersionSuffix is also misnamed. I'm re-titling the issue from "VersionPrefix is misnamed" to "VersionPrefix and VersionSuffix are misnamed".

Copied from original issue: dotnet/msbuild#1952

[rainersigwald]

From @tpluscode on April 7, 2017 7:14

The naming used here should follow the semver, right?

According to that spec, both 1.2.3 and 2.4.5-beta.2 are versions. The latter being a pre-release version in which what you called VersionSuffix is referred to as dot separated identifiers.

GitVersion uses the term pre-release tag although I'm not sure whether they coined that term. @asbjornu?

[rainersigwald]

From @asbjornu on April 7, 2017 7:29

I think @JakeGinnivan might know better than me where pre-release-tag came from, but as discussed in GitTools/GitVersion#1054, we might change tag to label or identifier (as seems to be the word used by semver) to not confuse it with a git tag.

[rainersigwald]

From @dasMulli on April 7, 2017 7:53

fyi the logic comes from https://github.com/dotnet/sdk/blob/master/src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.NET.DefaultAssemblyInfo.targets#L18-L22

It's hard since you need to be able to construct the Version from two parts. Changing the version based on the presence of a VersionSuffix isn't a good option here because it may be a global property and can't be overwritten.
So i guess the feature request would be to create an alias for VersionPrefix (e.g. VersionNumber - but i'm horrible at naming things)

[rainersigwald]

From @adamralph on April 8, 2017 6:37

Based on the comments above, it seems that the general opinion is that "1.0.0-beta1" is the "version", implying that VersionSuffix is also misnamed. I'm re-titling the issue from "VersionPrefix is misnamed" to "VersionPrefix and VersionSuffix are misnamed".

[rainersigwald]

From @adamralph on April 8, 2017 6:42

Unfortunately SemVer doesn't define terms to describe the "1.0.0" and "beta1" parts of the version "1.0.0-beta1" so I guess we'll have to define them (or look for precedents elsewhere).

But, before going too far down that path, perhaps we should challenge

you need to be able to construct the Version from two parts

Why?

[rainersigwald]

From @dasMulli on April 8, 2017 13:48

you need to be able to construct the Version from two parts

My reasoning is mostly based on the ability and need to pass version-related properties as arguments to msbuild invocations, e.g. from CI.
However, the importance of the different usage scenarios need to be weighted against each other.. Let me elaborate:

You can currently pass /p:VersionSuffix=beta1 and it will generate a 1.0.0-beta1 version (the dotnet cli even has convenience parameters for that). if you set /p:Version=1.2.3;VersionSuffix=beta1, you end up with 1.2.3 - which is expected and you get the same behaviour if you specified these two properties inside a project file or from command line. While not perfectly named, it is at least consistent.

However, take the following project file for example:

<Project>
  <PropertyGroup>
    <Version Condition="'$(Version)' == ''">1.0.0</Version>
    <Version Condition="'$(PreReleaseLabel)' != ''">$(Version)-$(PreReleaseLabel)</Version>
  </PropertyGroup>
  <Target Name="Build">
    <Message Importance="high" Text="Version: $(Version)"/>
  </Target>
</Project>

1. Invoke it with dotnet msbuild /p:PreReleaseLabel=beta1 and it prints 1.0.0-beta1.
2. Invoke it using dotnet msbuild /p:Version=1.2.3;PreReleaseLabel=beta1 and you get 1.2.3, because Version is now a global property and can't be overwritten.
3. Put a <Version>1.2.3</Version> at the top of the property group (pretend everything else is coming from the SDK) and run dotnet msbuild /p:PreReleaseLabel=beta1. The result will be 1.2.3-beta1

My problem with this is that 2. and 3. look similar - both properties set to a value - but behave differently because of the way global msbuild properties work, which is confusing for beginners trying to set up their project / CI.

There are a few workarounds for that but they would require a different property (ResolvedVersion) or each target that needs a version had to look at all available properties. (Not to mention this would be a breaking tooling change).

Summed up, I think it's best to leave Version being composed from two properties, but maybe figure out better names (falling back to the current ones).

[rainersigwald]

From @adamralph on April 8, 2017 17:7

if you set /p:Version=1.2.3;VersionSuffix=beta1, you end up with 1.2.3...

Therein lies the problem. If I pass Foo, and FooSuffix, I expected the value of Foo to be suffixed by the value of FooSuffix. The current behaviour is surprising and does not follow the semantics conveyed by the property names.

• which is expected...

It is only expected after you have experienced the surprising behaviour or read the documentation, and you have retained and recalled that knowledge before the occasion. The names (semantics) cannot be relied on to remind the user of the expected behaviour.

I still haven't seen a defense of the assertion that we need the version to be composed of two properties. What does this offer over a single property named Version?

[rainersigwald]

From @asbjornu on April 19, 2017 13:47

@adamralph:

Unfortunately SemVer doesn't define terms to describe the "1.0.0" and "beta1" parts of the version "1.0.0-beta1" so I guess we'll have to define them (or look for precedents elsewhere).

Yes, SemVer defines them as such:

Additional labels for pre-release and build metadata are available as extensions to the MAJOR.MINOR.PATCH format.

And:

A pre-release version MAY be denoted by appending a hyphen and a series of dot separated identifiers immediately following the patch version.

So SemVer calls these pre-release strings that are suffixed to the version for labels or identifiers. I would call the entire string for SemanticVersion, the version number for VersionNumber and the pre-release suffix for PreReleaseLabel.

[rainersigwald]

From @adamralph on April 19, 2017 15:57

@asbjornu defining SemanticVersion as {VersionNumber}-{PreReleaseLabel} is not a bad idea. For completeness, I would extend it to {VersionNumber}-{PreReleaseLabel}+{BuildLabel}.

However, I still challenge the need for multiple properties. I haven't seen a strong argument for having multiple properties over a single Version property.

[nguerrera]

We debated this quite a bit in #93 and didn't reach a consensus. I personally favored the single Version property approach as well. Still, we shouldn't break compatibility with projects that already use the VersionPrefix/VersionSuffix despite their less than perfect names.

The options here seem to be:

1. Pick better names. To remain compatible, we would have to support old names and new names, which I think would only make the situation even more confusing.

2. Have only the single Version property. This can't be done compatibly. In retrospect, I probably should have pushed harder on this before it was too late. However, you are already free to avoid VersionPrefix and VersionSuffix if you dislike them. The examples using only Version should work already.

I'm just not seeing a compelling enough reason to make a breaking change here and I don't see a non-breaking proposal that adds enough value. I'm closing this because I think the status quo is the least bad option we have.

[adamralph]

@nguerrera I hadn't even noticed that there was a single Version property already. The problem is, I've seen people use VersionSuffix alone, redundantly, in place of Version, which illustrates the problem. I.e. if there's no Version what can VersionSuffix suffix? That scenario should result in an exception being thrown somewhere.

I understand your reluctance to make breaking changes and I can understand the lack of appetite to make any changes to this now. Out of interest, is there any path for deprecation of properties, with or without eventual removal?

[jzabroski]

<Project>
  <PropertyGroup>
    <Version Condition="'$(Version)' == ''">1.0.0</Version>
    <Version Condition="'$(PreReleaseLabel)' != ''">$(Version)-$(PreReleaseLabel)</Version>
  </PropertyGroup>
  <Target Name="Build">
    <Message Importance="high" Text="Version: $(Version)"/>
  </Target>
</Project>

I'm a little sad we didn't go with this approach, extended to build labels, to match http://semver.org Backus-Naur Form Grammar specification.

I guess the upside is that this system is a bit more general than semver 2.0 and should there ever be a semver 3.0 this design will look brilliant,.

[jzabroski]

@mmitche Just wondering if the reason this says you referenced this issue is because of dotnet org merging repositories and github incorrectly linking to the wrong issue?

[mmitche]

@mmitche Just wondering if the reason this says you referenced this issue is because of dotnet org merging repositories and github incorrectly linking to the wrong issue?

Possibly because of merges, but where did mention it?

[jzabroski]

Look just below the comment by adamralph. #1131 (comment)

I've seen the arcade stuff falsely reference stuff a couple times and have always wondered WTF is going on. My guess is nobody is doing that on purpose and it's probably a github bug when repos merge because the log messages probably don't update.

[mmitche]

Yeah I think this is a repo merge issue.