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

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

Closed
melcloud opened this issue Oct 31, 2018 · 24 comments · Fixed by #2157
Closed

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

melcloud opened this issue Oct 31, 2018 · 24 comments · Fixed by #2157

Comments

@melcloud
Copy link

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
Copy link
Contributor

Skoucail commented May 27, 2019

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
Copy link
Contributor

Skoucail commented Jun 4, 2019

@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
Copy link
Member

Fixed by #1704.

@andrewescutia
Copy link

andrewescutia commented Jul 24, 2019

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member

asbjornu commented Jan 2, 2020

Can you see anything wrong with the regex FormatMetaDataPart?

private string FormatMetaDataPart(string value)
{
if(!string.IsNullOrEmpty(value))
value = Regex.Replace(value, "[^0-9A-Za-z-.]", "-");
return value;
}

@asbjornu asbjornu reopened this Jan 2, 2020
@Skoucail
Copy link
Contributor

Skoucail commented Jan 2, 2020

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
Copy link
Contributor

KiLLeRRaT commented Jan 2, 2020

I have created this PR that resolves the issue for the BranchName variable: #2008

This seems to do exactly what I need.

@Skoucail
Copy link
Contributor

Skoucail commented Jan 2, 2020

@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
Copy link
Contributor

Can you see anything wrong with the regex FormatMetaDataPart?

private string FormatMetaDataPart(string value)
{
if(!string.IsNullOrEmpty(value))
value = Regex.Replace(value, "[^0-9A-Za-z-.]", "-");
return value;
}

@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

public string BranchName => semver.BuildMetaData.Branch;
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
Copy link
Contributor

KiLLeRRaT commented Jan 2, 2020

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

I'm guessing we could do something like wrap

public string BranchName => semver.BuildMetaData.Branch;
with a FormatMetaDataPart e.g.:

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

I haven't tried this though.

@Skoucail
Copy link
Contributor

Skoucail commented Jan 2, 2020

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
Copy link
Contributor

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
Copy link
Contributor

Skoucail commented Jan 3, 2020

@KiLLeRRaT

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

var semverFormatValues = new SemanticVersionFormatValues(semanticVersion, config);
var informationalVersion = CheckAndFormatString(config.AssemblyInformationalFormat, semverFormatValues,
environment, semverFormatValues.DefaultInformationalVersion, "AssemblyInformationalVersion");

The code for semverFormatValues.DefaultInformationalVersion is:

public string DefaultInformationalVersion => semver.ToString("i");

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).

private static string CheckAndFormatString<T>(string formatString, T source, IEnvironment environment, string defaultValue, string formatVarName)
{
string formattedString;
if (string.IsNullOrEmpty(formatString))
{
formattedString = defaultValue;
}
else
{
try
{
formattedString = formatString.FormatWith(source, environment);
}
catch (ArgumentException formex)
{
throw new WarningException($"Unable to format {formatVarName}. Check your format string: {formex.Message}");
}
}
return formattedString;
}

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
Copy link
Contributor

Skoucail commented Jan 3, 2020

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
Copy link
Member

asbjornu commented Jan 3, 2020

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 pushed a commit to Skoucail/GitVersion that referenced this issue Jan 3, 2020
@Skoucail
Copy link
Contributor

Skoucail commented Jan 3, 2020

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
Copy link
Contributor

Thanks everyone for helping with this. Much appreciated.

@Skoucail
Copy link
Contributor

Skoucail commented Jan 7, 2020

@KiLLeRRaT your welcome

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

@asbjornu
Copy link
Member

@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
Copy link
Contributor

KiLLeRRaT commented Mar 4, 2020

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
Copy link
Member

asbjornu commented Mar 5, 2020

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
Copy link
Contributor

Here you go: #2157

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 a pull request may close this issue.

5 participants