Fixing https://github.com/googlesamples/unity-jar-resolver/issues/48 #49
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
|
I signed it! |
yuriy-universal-ivanov
force-pushed
the
issue-48
branch
from
ca490f5 to
f244e7d
Compare
|
CLAs look good, thanks! |
| @@ -1038,7 +1036,10 @@ public void ClearDependencies() | |||
| KeyValuePair<Dependency, string> oldDepFilenamePair; | |||
| if (currentDepsByVersionlessKey.TryGetValue(dep.VersionlessKey, | |||
| out oldDepFilenamePair)) { | |||
| if (oldDepFilenamePair.Key.BestVersion != dep.BestVersion && | |||
| string oldVersion = ExtractVersionFromFileName( | |||
There was a problem hiding this comment.
Dependency.BestVersion should be the version string so in both cases this will be either a version string or an empty string see https://github.com/googlesamples/unity-jar-resolver/blob/master/source/JarResolverLib/src/Google.JarResolver/Dependency.cs#L158
Why did you need to change this to extract the version from the filename? Are you seeing a file name sneak into this version field?
| filenameWithoutExtension.Substring( | ||
| dep.Artifact.Length + 1), "^([0-9.]+)"); | ||
| if (match.Success) | ||
| string artifactVersion = ExtractVersionFromFileName( |
There was a problem hiding this comment.
Not sure why you need this. The previous code attempted to extract the version from the filename and if the match succeeds then jumps into the block below and pulls the version from match.Groups[1].Value. Did you see a problem in this code path or was this just a clean up?
| string oldVersion = ExtractVersionFromFileName( | ||
| oldDepFilenamePair.Key.BestVersion); | ||
| string newVersion = ExtractVersionFromFileName(dep.BestVersion); | ||
| if ((oldVersion == null || (newVersion != null && oldVersion != newVersion)) && | ||
| (confirmer == null || confirmer(oldDepFilenamePair.Key, dep))) { |
There was a problem hiding this comment.
The stack trace reported in bug #48 starts here and bubbles up through to Resolver1_1.AarPathToPackageName where - from your report - the filename was null and therefore caused a null reference exception. Which suggests https://github.com/googlesamples/unity-jar-resolver/blob/master/source/PlayServicesResolver/src/ResolverVer1_1.cs#L723 is returning a null path (as artifactPath) instead.
There was a problem hiding this comment.
Thanks for the patch.
Though, I don't see how this fixes #48 given the reported stack trace. Could you add more details to the commit message that describes what the problem is and how you fixed it?
|
See #48 (comment) |
|
Ah ok, I see now. Any chance you could update the commit message to reflect the comment you added here #48 (comment) ? |
Using the same rule (and the same function) for extracting a version code when comparing libraries versions, as used to generate file names when storing them in Plugins/Android folder, so repetitive checks for versions with -alpha like postfixes don't misdetect them as a new version each time.
yuriy-universal-ivanov
force-pushed
the
issue-48
branch
from
f244e7d to
e49d009
Compare
|
@stewartmiles , |
|
Thank you. |
Fix for #48