search

GitTools / GitVersion

From git log to SemVer in no time
https://gitversion.net/docs/
MIT License
2.85k stars 650 forks source link

Bug: Branch contains / is not replaced by - in build metadata #1520

Closed melcloud closed 4 years ago

melcloud commented 5 years ago

GitVersion version: 4.0, 3.6.4 Problem: Given a branch name contains /, e.g. hotfixes/build-script, the build metadata does not convert it to hyphen. This is not a valid semver 2.

{
  "Major":0,
  "Minor":2,
  "Patch":1,
  "PreReleaseTag":"beta.1",
  "PreReleaseTagWithDash":"-beta.1",
  "PreReleaseLabel":"beta",
  "PreReleaseNumber":1,
  "BuildMetaData":11,
  "BuildMetaDataPadded":"0011",
  "FullBuildMetaData":"11.Branch.hotfixes/build-script.Sha.301ff2de375da97133ab544c9561cbd909039877",
  "MajorMinorPatch":"0.2.1",
  "SemVer":"0.2.1-beta.1",
  "LegacySemVer":"0.2.1-beta1",
  "LegacySemVerPadded":"0.2.1-beta0001",
  "AssemblySemVer":"0.2.1.1",
  "FullSemVer":"0.2.1-beta.1+11",
  "InformationalVersion":"0.2.1-beta.1+11.Branch.hotfixes/build-script.Sha.301ff2de375da97133ab544c9561cbd909039877",
  "BranchName":"hotfixes/build-script",
  "Sha":"301ff2de375da97133ab544c9561cbd909039877",
  "NuGetVersionV2":"0.2.1-beta0001",
  "NuGetVersion":"0.2.1-beta0001",
  "CommitsSinceVersionSource":11,
  "CommitsSinceVersionSourcePadded":"0011",
  "CommitDate":"2018-10-31"
}
Skoucail commented 5 years ago

Any update on this?

NuGet 4.3.0 (and higher with support for SemVer 2.0) actually has issues with this. Nuget pack defaults to version 1.0.0 when AssemblyInformationalVersion metadata contains forward slash Nuget pack with invalid AssemblyInformationalVersion value reports "description is required"

And according to the Semantic Versioning 2.0.0 spec '/' isn't an allowed character.

Build metadata MAY be denoted by appending a plus sign and a series of dot separated identifiers immediately following the patch or pre-release version. Identifiers MUST comprise only ASCII alphanumerics and hyphen [0-9A-Za-z-]. Identifiers MUST NOT be empty. Build metadata SHOULD be ignored when determining version precedence. Thus two versions that differ only in the build metadata, have the same precedence. Examples: 1.0.0-alpha+001, 1.0.0+20130313144700, 1.0.0-beta+exp.sha.5114f85.

Skoucail commented 5 years ago

@arturcic i'm not sure if you are the correct person to tag into this. But for the moment you seem to be the most active in this repo. Are you able to take a look at this? This is kinda a breaking issue if we want to use full SemVer2.0 with NuGet (or other tools) that implement the specification.

asbjornu commented 5 years ago

Fixed by #1704.

andrewescutia commented 5 years ago

this still seems to be an issue if you overwrite the assembly-informational-format. Take the following format defined in the yml: {MajorMinorPatch}{PreReleaseTagWithDash}+{BranchName}.{ShortSha}

I get the following: "InformationalVersion":"4.5.1-beta.2+feature/git-version-2.41f34da",

Skoucail commented 5 years ago

The fix was included in version 5.0.0. This version has only been released 14d ago. Are you sure you tested the issue with a pre-release version of v5.0.0 and not with v4.x.x?

KiLLeRRaT commented 4 years ago

I'm still getting this issue as mentioned by @andrewescutia.

I'm running GitVersion version 5.1.2 through MSBuild.

I have the following in my GitVersion.yml:

assembly-informational-format: '{Major}.{Minor}.{Patch}+{CommitsSinceVersionSource}.Branch.{BranchName}.Sha.{ShortSha}'

The output is:

19.12.26+1.Branch.albert/test1.Sha.f5a0018

What I'm really trying to achieve is to have the exact same assembly-informational-format as the default one, but with a short Sha because the long Sha is giving path length trouble for me....

asbjornu commented 4 years ago

Can you see anything wrong with the regex FormatMetaDataPart?

https://github.com/GitTools/GitVersion/blob/5d6ed2ae3c03ea3f0299e631076cf145731daa2d/src/GitVersionCore/SemanticVersioning/SemanticVersionBuildMetaData.cs#L163-L168

Skoucail commented 4 years ago

The think confusion is between the SemanticVersion and the other variables (in this case InformationalVersion). The SemanticVersion spec doesn't allow certain characters. So that got fixed. The other fields in the GitVersion output stayed untouched.

At first glance i don't believe there is a configuration option to change the SemanticVerion format.

KiLLeRRaT commented 4 years ago

I have created this PR that resolves the issue for the BranchName variable: https://github.com/GitTools/GitVersion/pull/2008

This seems to do exactly what I need.

Skoucail commented 4 years ago

@KiLLeRRaT I commented on the pull request. I don’t think the change is a good approach of the problem.

If i understand it correctly the problem is that you use the informational assembly version as SemVer because you need a shorter version of the default SemVer outputted by GitVersion.

The correct extension would be to add a configuration setting so the SemVersion format can be changed. And in the SemVer special chars aren’t allowed and get replaces by dashes.

KiLLeRRaT commented 4 years ago

Can you see anything wrong with the regex FormatMetaDataPart?

https://github.com/GitTools/GitVersion/blob/5d6ed2ae3c03ea3f0299e631076cf145731daa2d/src/GitVersionCore/SemanticVersioning/SemanticVersionBuildMetaData.cs#L163-L168

@asbjornu I don't see anything wrong with the RegEx.

I don't think this method is called for the BranchName variable though.

If you look at https://github.com/GitTools/GitVersion/blob/5d6ed2ae3c03ea3f0299e631076cf145731daa2d/src/GitVersionCore/SemanticVersioning/SemanticVersionFormatValues.cs#L54 you will see that it doens't to a .ToString("xxxxxx") on it. It just grabs the value.

The only place the code referenced by @asbjornu is called in from within this ToString method.

KiLLeRRaT commented 4 years ago

@Skoucail, your comment makes sense. That leaves us with this problem though.

I'm guessing we could do something like wrap https://github.com/GitTools/GitVersion/blob/5d6ed2ae3c03ea3f0299e631076cf145731daa2d/src/GitVersionCore/SemanticVersioning/SemanticVersionFormatValues.cs#L54 with a FormatMetaDataPart e.g.:

public string BranchName => FormatMetaDataPart(semver.BuildMetaData.Branch);

I haven't tried this though.

Skoucail commented 4 years ago

Well depends on the opinions i guess. As mentioned in the pull request: i believe changing output variables like BranchName will break some people (build) systems. And i think there is no reason why the variable BranchName should’t contain special characters.

Only the outputted SemVer isn’t allowed to contain special slaches.

The thing missing in GitVersion is the ability to set the SemVer format. While the informational version format can be set but doesn’t impact the generated SemVer.

KiLLeRRaT commented 4 years ago

Hi @Skoucail ,

Is the SemVer that is set agains the assembly coming straight from assembly-informational-format?

If I change that format e.g.: 

assembly-informational-format: '{Major}.{Minor}.{Patch}+{CommitsSinceVersionSource}.Branch.{BranchName}.Sha.{ShortSha}'

then my output dll contains that.

Are you saying that we should implement another configurable format and output, so instead of using assembly-informational-format for the SemVer set against the dll, we use a new one, e.g. semver-format which is escaped etc.

The assembly-informational-format will still exist but is then to be used for other purposes potentially?

Cheers,

Skoucail commented 4 years ago

@KiLLeRRaT

Sorry i was a bit confused yesterday. I think i just found the code where the behavior changes.

https://github.com/GitTools/GitVersion/blob/c2005e172e1106d9d8c399baa191a05d9dea2284/src/GitVersionCore/OutputVariables/VariableProvider.cs#L56-L59

The code for semverFormatValues.DefaultInformationalVersion is: https://github.com/GitTools/GitVersion/blob/c2005e172e1106d9d8c399baa191a05d9dea2284/src/GitVersionCore/SemanticVersioning/SemanticVersionFormatValues.cs#L72

So if the default is used they use the ToString function that got changed/fixed in my pull request. But the CheckAndFormatString function with a formatString just paste together other properties (like BranchName).

https://github.com/GitTools/GitVersion/blob/c2005e172e1106d9d8c399baa191a05d9dea2284/src/GitVersionCore/OutputVariables/VariableProvider.cs#L110-L131

I still believe changing the BranchName to replace special characters with daches would for some people break their systems/setups. So we need a more sophisticated fix for this issue that only affects the InformationalVersion.

Skoucail commented 4 years ago

I believe the fix with the least impact is adding RegexReplace("[^0-9A-Za-z-.+]", "-") to the result of CheckAndFormatString for informationalVersion. Any thoughts?

var informationalVersion = CheckAndFormatString(config.AssemblyInformationalFormat, semverFormatValues, environment, semverFormatValues.DefaultInformationalVersion, "AssemblyInformationalVersion").RegexReplace("[^0-9A-Za-z-.+]", "-");

asbjornu commented 4 years ago

I agree that sounds like the most narrow-scoped solution, @Skoucail. Would you or @KiLLeRRaT be up for a PR that implements this with at least one test that ensures the reported bug is fixed?

Skoucail commented 4 years ago

I agree that sounds like the most narrow-scoped solution, @Skoucail. Would you or @KiLLeRRaT be up for a PR that implements this with at least one test that ensures the reported bug is fixed?

I already made some tests to debug the issue. The pull request is created with the tests and the fix. (#2014)

KiLLeRRaT commented 4 years ago

Thanks everyone for helping with this. Much appreciated.

Skoucail commented 4 years ago

@KiLLeRRaT your welcome

@asbjornu Do you have an idea when this fix will be released to stable?

asbjornu commented 4 years ago

@Skoucail, we don't plan releases, but I suppose we could get a v5.1.4 or 5.2 out the door some time soon.

KiLLeRRaT commented 4 years ago

Hi Guys,

I could finally test this after other build issues were merged in.

I'm using 5.1.4-beta1.220

I have the following set in my GitVersion.yml:

assembly-informational-format: '{Major}.{Minor}.{Patch}+{CommitsSinceVersionSource}.Branch.{BranchName}.Sha.{ShortSha}'

This still outputs with a slash in the branch name

InformationalVersion: 20.1.24+7.Branch.feature/20200224_UpgradeGitVersionTask.Sha.6285b41

Here is the full output:

Major: 20
Minor: 1
Patch: 24
PreReleaseTag: 20200224-UpgradeGitVersionTask.1
PreReleaseTagWithDash: -20200224-UpgradeGitVersionTask.1
PreReleaseLabel: 20200224-UpgradeGitVersionTask
PreReleaseNumber: 1
WeightedPreReleaseNumber: 30001
BuildMetaData: 7
BuildMetaDataPadded: 0007
FullBuildMetaData: 7.Branch.feature-20200224-UpgradeGitVersionTask.Sha.6285b41097704a6a585c06f7a0247ff6d654d738
MajorMinorPatch: 20.1.24
SemVer: 20.1.24-20200224-UpgradeGitVersionTask.1
LegacySemVer: 20.1.24-20200224-UpgradeGit1
LegacySemVerPadded: 20.1.24-20200224-Upgrade0001
AssemblySemVer: 20.0.0.0
AssemblySemFileVer: 20.1.24.0
FullSemVer: 20.1.24-20200224-UpgradeGitVersionTask.1+7
InformationalVersion: 20.1.24+7.Branch.feature/20200224_UpgradeGitVersionTask.Sha.6285b41
BranchName: feature/20200224_UpgradeGitVersionTask
Sha: 6285b41097704a6a585c06f7a0247ff6d654d738
ShortSha: 6285b41
NuGetVersionV2: 20.1.24-20200224-upgrade0001
NuGetVersion: 20.1.24-20200224-upgrade0001
NuGetPreReleaseTagV2: 20200224-upgrade0001
NuGetPreReleaseTag: 20200224-upgrade0001
VersionSourceSha: 5c6d1c3fa8aca9b478c5dc14e14cb10f20aa5950
CommitsSinceVersionSource: 7
CommitsSinceVersionSourcePadded: 0007
CommitDate: 2020-03-04

If I run it without the custom format in GitVersion.yml then I get:

InformationalVersion: 20.1.24-20200224-UpgradeGitVersionTask.1+7.Branch.feature-20200224-UpgradeGitVersionTask.Sha.6285b41097704a6a585c06f7a0247ff6d654d738

The above is correct, but way too long and Azure Pipelines/Octopus Deploy complains that the version string is too long.

As mentioned earlier, I need to make my own assembly-informational-format because the default one is too long.

I still need to get a branch name that doesn't contain a slash from somewhere.

Any ideas?

Should I just implement another variable called BranchNameEscaped or something?

Cheers

asbjornu commented 4 years ago

It would be wrong to modify BranchName, so I suppose we can add a new variable for this which is both exposed in the JSON as well as used as the current escaped branch name inside the other variables. In WeightedPreReleaseNumber the adjective is prefixed, so EscapedBranchName or something similar, perhaps?

KiLLeRRaT commented 4 years ago

Here you go: https://github.com/GitTools/GitVersion/pull/2157