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

Adjust the algorithm in GetVersionForSingleSegment #7769

Merged

Conversation

michellemcdaniel
Copy link
Contributor

@michellemcdaniel michellemcdaniel commented Aug 19, 2021

GetVersionForSingleSegment assumes that if we find a fully formed major.minor.patch in the name of a file, followed by something that is not part of the version, that is definitely the version of the file. However, there are instances where there are two well formed major.minor.patch versions in the file name. For example: Microsoft.NET.Workload.Mono.ToolChain.Manifest-6.0.100.Msi.x64.6.0.0-rc.1.21380.2.symbols.nupkg. Mono puts the Sdk version in the name of their packages, in addition to the version of the package itself. GetVersionForSingleSegment sees the 6.0.100 followed by .Msi, recognizes that .Msi is not part of a major.minor.patch, and returns 6.0.100, without considering that there is considerably more in the file name that should be considered.

This change adds a dictionary to track every potential version in a segment, and where in the segment the potential version was found. It then will return the version that occurs the latest in the segment.

To double check:

    GetVersionForSingleSegment assumes that if we find a fully formed major.minor.patch in the name of a file, followed by something that is not part of the version, that is definitely the version of the file. However, there are instances where there are two well formed major.minor.patch versions in the file name. For example: `Microsoft.NET.Workload.Mono.ToolChain.Manifest-6.0.100.Msi.x64.6.0.0-rc.1.21380.2.symbols.nupkg`. Mono puts the Sdk version in the name of their packages, in addition to the version of the package itself. GetVersionForSingleSegment sees the 6.0.100 followed by .Msi, recognizes that .Msi is not part of a major.minor.patch, and returns 6.0.100, without considering that there is considerably more in the file name that should be considered.

    This change adds a dictionary to track every potential version in a segment, and where in the segment the potential version was found. It then will return the version that occurs the latest in the segment.
Copy link
Member

@ChadNedzlek ChadNedzlek left a comment

Choose a reason for hiding this comment

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

I... think this works? It's shockingly complicated code to find some numbers in a string, but I think I understand it. I'm not sure what order things are in, but I feel like the dictionary probably isn't necessary, since it's always just the last one that's been found, so you could record that? But I could be wrong.

@michellemcdaniel
Copy link
Contributor Author

I did the dictionary thing because of Microsoft.NET.Workload.Mono.ToolChain.Manifest-6.0.100.6.0.0-preview.7.21377.20.symbols.nupkg. Because we are using a stack, 6,0,100 gets pushed onto the stack, but we keep pushing 6,0,0 until we see the -preview. In the old algorithm, we would just return 6.0.0-preview.7.21377.20, but since we need to keep looking for other cases, we save that away. At that point, the algorithm sees there's nothing left to find, but also that 6.0.100 is still on the stack, pops it, and would save that as a "good" version. If we don't have the dictionary/where in the string we found it, we would either naively return one of the versions we found, which would be wrong for something, depending on what we returned. Hence, the dictionary with locations.

@michellemcdaniel michellemcdaniel merged commit 23372e9 into dotnet:main Aug 19, 2021
mmitche added a commit to mmitche/arcade-services that referenced this pull request Sep 13, 2021
This pulls in the fix made here dotnet/arcade#7769, which is necessary for an internal build, which pushes symbol packages to a nuget feed. The version identification is critical there because the version is a separate element of the azdo download path. This contrasts with public builds, where symbol packages end up in blob storage and no version identification is necessary.
mmitche added a commit to dotnet/arcade-services that referenced this pull request Sep 13, 2021
This pulls in the fix made here dotnet/arcade#7769, which is necessary for an internal build, which pushes symbol packages to a nuget feed. The version identification is critical there because the version is a separate element of the azdo download path. This contrasts with public builds, where symbol packages end up in blob storage and no version identification is necessary.
@epananth epananth mentioned this pull request Oct 21, 2021
1 task
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.

4 participants