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

fix: handle General/Complex Versioning in --bump #2889

Merged
merged 6 commits into from
Nov 29, 2024
Merged

Conversation

liskin
Copy link
Contributor

@liskin liskin commented Nov 1, 2024

fix: handle General/Complex Versioning in --bump

The previous fix to handle non-semver versions in --bump used a crude heuristic - a check whether there's a "." in the version string - to decide whether to attempt to chunkify or just fall back to returning the new version unmodified. This, however, resulted in no bump happening (None returned from check_semver_bump) when either of the old/new versions looked like "v1.2.3".

Additionally, versions like "1.2a1" and "1.2a2" were considered the same, because: a) the heuristic failed to recognise they won't be parsed/chunkified correctly, and b) chunkify used nth that silently throws parts of the version away.

I'm proposing to fix this by reimplementing chunkify using the much more generic versions::Mess format, which simply splits the version number into chunks and separators. We then convert it into a simpler format of just chunks (first chunk as is, further chunks with a separator prepended).

This design has an issue: we don't recognise a change in the versioning scheme. A bump from "<commit sha>" to "v1.2.3" will result in just "v1", because the commit sha is parsed as a single chunk. The most obvious case of this, "latest" being parsed as a single chunk, is handled explicitly in the code, as it would just be a mess otherwise.

A potential workaround for this issue would be to add a flag (e.g. --pin) that would make --bump skip the check_semver_bump logic and always use the full new version (as suggested in #2704 (comment)). This would also help in the following case: a project using variable length version numbers instead of the full 3-chunk semver. Trying to follow this sequence of bumps: "20.0", "20.0.1", "20.1" isn't possible with the current logic.

Related: 0b2c2aa ("fix: upgrade --bump with non-semver versions (#2809)")

fix: Allow --bump from 20.0.1 to 20.1

It's weird so we still warn, but returning None from check_semver_bump only makes sense if the versions are deemed to be the same. Otherwise it's just confusion for the user — the UI presents this as an upgrade, proceeds to uninstall the old version, but fails to do the actual bump and no new version is installed.

@liskin
Copy link
Contributor Author

liskin commented Nov 1, 2024

This design has an issue: we don't recognise a change in the versioning scheme. A bump from "<commit sha>" to "v1.2.3" will result in just "v1", because the commit sha is parsed as a single chunk. The most obvious case of this, "latest" being parsed as a single chunk, is handled explicitly in the code, as it would just be a mess otherwise.

The tests caught this, precisely because it now tries to bump dummy from "ref:master" to "2.0" (the new/latest version being "2.0.0").

I suspect that a proper solution would require us to detect whether the two versions follow the same/similar versioning scheme, and don't try to do check_semver_bump if old is something like "ref:4a2d75f…" or "prefix:20". It's not trivial, though, as there's some inconsistency here – the pipx/uv backend doesn't support ref: or rev: at all, for example, it expects to just get the commit sha directly. And also, list_outdated_versions tries to some prefix stripping magic—although maybe that's not necessary now that we handle non-semver versions?

Any ideas?

@jdx
Copy link
Owner

jdx commented Nov 1, 2024

I think right now it probably doesn't work right with ref: or prefix: so we should probably just ignore those in mise up altogether, maybe with a warning, until we have a better solution for them.

check_semver_bump("20.0a1", "20.0a2"),
Some("20.0a2".to_string())
);
std::assert_eq!(check_semver_bump("v20", "v20.0.0"), None);
Copy link
Owner

Choose a reason for hiding this comment

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

are you actually seeing v prefixed versions somewhere? we should be removing that prefix everywhere we can by this point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They're in my config so I thought it'd be nice if mise upgrade --bump didn't choke on them. I can remove them manually but it'd be nice if others didn't need to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But you've got a point that the test cases don't reflect reality. There might be a "v" in old but not in new.

@liskin
Copy link
Contributor Author

liskin commented Nov 1, 2024

I think right now it probably doesn't work right with ref: or prefix: so we should probably just ignore those in mise up altogether, maybe with a warning, until we have a better solution for them.

Actually, the code already kinda tries to ignore them:

mise/src/toolset/mod.rs

Lines 402 to 420 in 9bedf17

match out.tool_request.clone() {
ToolRequest::Version {
backend,
version: _version,
options,
source,
} => {
out.tool_request = ToolRequest::Version {
backend,
options,
source,
version: out.bump.clone().unwrap(),
};
}
_ => {
warn!("upgrading non-version tool requests");
out.bump = None;
}
}

But it's half-assed — only skips the bump, but still performs the uninstall. And then, as I mentioned earlier in #2704 (comment), the dry-run output doesn't match what's actually going to happen, so the output in the failing test is "Would install [email protected]" but that's not what would happen in practice, as there'd be no bump.

Oh, what a can of worms. I think I give up for today :-D

@jdx
Copy link
Owner

jdx commented Nov 16, 2024

I did some work around prefix: in #3054

The previous fix to handle non-semver versions in `--bump` used a crude
heuristic - a check whether there's a "." in the version string - to
decide whether to attempt to `chunkify` or just fall back to returning
the new version unmodified. This, however, resulted in no bump happening
(`None` returned from `check_semver_bump`) when either of the old/new
versions looked like "v1.2.3".

Additionally, versions like "1.2a1" and "1.2a2" were considered the
same, because: a) the heuristic failed to recognise they won't be
parsed/chunkified correctly, and b) `chunkify` used `nth` that silently
throws parts of the version away.

I'm proposing to fix this by reimplementing `chunkify` using the much
more generic versions::Mess format, which simply splits the version
number into chunks and separators. We then convert it into a simpler
format of just chunks (first chunk as is, further chunks with a
separator prepended).

This design has an issue: we don't recognise a change in the versioning
scheme. A bump from "<commit sha>" to "v1.2.3" will result in just "v1",
because the commit sha is parsed as a single chunk. The most obvious
case of this, "latest" being parsed as a single chunk, is handled
explicitly in the code, as it would just be a mess otherwise.

A potential workaround for this issue would be to add a flag (e.g.
`--pin`) that would make `--bump` skip the `check_semver_bump` logic and
always use the full new version (as suggested in
jdx#2704 (comment)).
This would also help in the following case: a project using variable
length version numbers instead of the full 3-chunk semver. Trying to
follow this sequence of bumps: "20.0", "20.0.1", "20.1" isn't possible
with the current logic.

Related: 0b2c2aa ("fix: upgrade --bump with non-semver versions (jdx#2809)")
It's weird so we still warn, but returning `None` from
`check_semver_bump` only makes sense if the versions are deemed to be
the same. Otherwise it's just confusion for the user — the UI presents
this as an upgrade, proceeds to uninstall the old version, but fails to
do the actual bump and no new version is installed.
@jdx jdx enabled auto-merge (squash) November 29, 2024 10:31
@jdx jdx merged commit e5efc7f into jdx:main Nov 29, 2024
19 checks passed
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.

2 participants