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

Version.Component.parse parses githash as separate components #3124

Closed
alexklibisz opened this issue Jul 17, 2023 · 2 comments · Fixed by #3260
Closed

Version.Component.parse parses githash as separate components #3124

alexklibisz opened this issue Jul 17, 2023 · 2 comments · Fixed by #3260
Labels
bug Something isn't working cat:version-selection

Comments

@alexklibisz
Copy link
Contributor

Hi, I hit an interesting case recently which I believe boils down a possible issue in Version.Component.parse (hence the title).

Say I have the following versions:

val currentVersion = Version("1.0.0+1319.ae77058")
val nextVersion = Version("1.0.0+1320.38b57aa")

The version suffix consists of a monotonically increasing build number and a githash.

I would expect currentVersion.selectNext(List(nextVersion) to return Some(nextVersion).

However, it actually returns None.

When I looked into this more, it turns out that this is returning None because of how nextVersion is parsed:

Version.Component.parse("1.0.0+1320.38b57aa").foreach(println(_))

returns:

Numeric(1)
Separator(.)
Numeric(0)
Separator(.)
Numeric(0)
Separator(+)
Numeric(1320)
Separator(.)
Numeric(38)
Alpha(b)
Numeric(57)
Alpha(aa)

Crucially, the segment .38b57aa is parsed into five components: Separator(.), Numeric(38), Alpha(b), Numeric(57), Alpha(aa)

Calling Alpha(b).order returns -4, because this component is getting interpreted as a "beta" component, at this line: https://github.com/scala-steward-org/scala-steward/blob/main/modules/core/src/main/scala/org/scalasteward/core/data/Version.scala#L171C5-L171C5

So then the nextVersion.minAlphaOrder returns -4, causing it to get filtered out at this line: https://github.com/scala-steward-org/scala-steward/blob/main/modules/core/src/main/scala/org/scalasteward/core/data/Version.scala#L70

This turns out to be the case for any first containing substrings m, ea, rc, etc.

For example, this ends up printing None:

import org.scalasteward.core.data.Version

val currentVersion = Version("1.0.0+1319.ae77058")
val nextVersions = List(
  Version("1.0.0+1320.38b57aa"),
  Version("1.0.0+1320.38m57aa"),
  Version("1.0.0+1320.38ea57aa")
)

println(currentVersion.selectNext(nextVersions)) // None

Some thoughts/observations:

I would have expected "38m57aa" to get parsed as a component, not a set of components.

It seems like scala-steward already knows that the dot character is a separator, it just doesn't know that "38m57aa" is a githash.

The component parser does have a sub-parser for "hash", but it's unclear whether this particular substring matches that pattern. I'm not familiar with the cats parser syntax, so I can't really interpret whether this is desired or a defect.

Even if this particular case was parsed "correctly" (by my interpretation), what happens when the githash component starts with a b or m or ea?

@alexklibisz
Copy link
Contributor Author

As a workaround, we have just altered the version to omit the git hashes.

fthomas added a commit that referenced this issue Jan 5, 2024
fthomas added a commit that referenced this issue Jan 5, 2024
This allows hashes to also be separated with a `.` and `_` from the
preceding component.

Here is for example what `Version.Component.parse("1.0.0+1320.38b57aa")`
returned before this change and what it returns now:
```
before:
Numeric(1), Separator(.), Numeric(0), Separator(.), Numeric(0), Separator(+),
Numeric(1320), Separator(.), Numeric(38), Alpha(b), Numeric(57), Alpha(aa)

now:
Numeric(1), Separator(.), Numeric(0), Separator(.), Numeric(0), Separator(+),
Numeric(1320), Separator(.), Hash(38b57aa))
```

Btw, recognizing hashes was added in #1549 but that PR gives no
explanation why they only could preceded by `-` and `+`.

Closes #3124
@fthomas fthomas added cat:version-selection bug Something isn't working labels Jan 5, 2024
@fthomas
Copy link
Member

fthomas commented Jan 5, 2024

Turns out this issue has a simple fix, namely allowing . to precede hashes, see #3260. We currently only recognize hashes if they are preceded by a - or +-

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cat:version-selection
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants