Skip to content

Fix parsing of GVFS-aware Git version numbers that contain - (for PR validation builds)#167

Merged
nickgra merged 4 commits intomicrosoft:masterfrom
nickgra:gitprbuild
Aug 16, 2018
Merged

Fix parsing of GVFS-aware Git version numbers that contain - (for PR validation builds)#167
nickgra merged 4 commits intomicrosoft:masterfrom
nickgra:gitprbuild

Conversation

@nickgra
Copy link
Member

@nickgra nickgra commented Aug 15, 2018

Correct regex used to match GVFS-aware Git package to actually match the NuGet spec for what defines a package. See https://docs.microsoft.com/en-us/nuget/reference/package-versioning

This fixes #175

@sanoursa
Copy link
Contributor

The fact that this same regex shows up in 3 separate scripts could end up being a maintenance issue. Can it be isolated into just one script that returns the appropriate version number?

$SCRIPTDIR/DownloadGVFSGit.sh || exit 1
GVFSPROPS=$SRCDIR/GVFS/GVFS.Build/GVFS.props
GITVERSION="$(cat $GVFSPROPS | grep GitPackageVersion | grep -Eo '[0-9.]{1,}')"
GITVERSION="$(cat $GVFSPROPS | grep GitPackageVersion | grep -Eo '[0-9.]+(-\w+)?')"
Copy link
Member

Choose a reason for hiding this comment

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

I'm not familiar with bash regex syntax, what does +(-\w+)? match against?

Copy link
Member Author

Choose a reason for hiding this comment

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

The - here is matching the a literal '-' character, which has to precede any text string in a NuGet version number (according to their spec).

The move from {1,} to + is from me realizing that egrep supports notation like + and ?, it should've been a + for readability to begin with.

Copy link
Member

Choose a reason for hiding this comment

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

What does the \w match? (also where did you find the spec?)

Copy link
Member Author

Choose a reason for hiding this comment

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

The \w matches any word (alphanumeric) character, so we're matching any number of words after the -. So -pr, -rc, -release1 would all be valid values, but something like -release-1 wouldn't be.

The 'spec' of sorts is here: https://docs.microsoft.com/en-us/nuget/reference/package-versioning

@nickgra
Copy link
Member Author

nickgra commented Aug 15, 2018

@sanoursa I agree that it's no good to have this code duplicated in three places. In the past we talked about writing out the version number once to the build directory and having other scripts just read it from that file. Our intention was to make sure that we weren't using a stale version number (imagine the version has changed since your last build, and you're running the Prep script before ever running the Build script that would create the file), but I'm not sure if that scenario is going to ever be common.

I can make that change here.

@sanoursa
Copy link
Contributor

@nickgra isn't it possible to call a script that returns a value?

@nickgra
Copy link
Member Author

nickgra commented Aug 16, 2018

@sanoursa Yup, that approach works too. I took that approach for commit 2.

@nickgra nickgra merged commit ecab6ac into microsoft:master Aug 16, 2018
@nickgra nickgra deleted the gitprbuild branch August 16, 2018 22:27
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.

GVFS-aware Git versions containing '-' don't get parsed correctly

3 participants