Unify daemon app version types

[kl]

Android now has alpha builds which currently crash because the daemon version parsing code does not handle version names with "alpha" in them.

This commit removes ParsedAppVersion in favor of mullvad_version::Version. This ensures that we have one place in the code base the parses version strings, and also fixes the issue where ParsedAppVersion didn't handle alpha versions, which mullvad_version::Version currenty supports.

---

This change is 

[linear]

DROID-1693 Alpha builds are crashing

[albin-mullvad]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @kl)

---

mullvad-types/src/version.rs line 16 at r1 (raw file):

// alpha builds, so the code in this file will simply parse alpha versions as beta versions, so
// as far as the Rust code is concerned there is no difference between -alpha and -beta.
static BETA_REGEX: LazyLock<Regex> = LazyLock::new(|| {

Shouldn't we rename this one to something like PRE_STABLE_REGEX to match the naming in mullvad-version?

Code quote:

BETA_REGEX

[kl]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @albin-mullvad)

---

mullvad-types/src/version.rs line 16 at r1 (raw file):

Previously, albin-mullvad wrote…

Shouldn't we rename this one to something like PRE_STABLE_REGEX to match the naming in mullvad-version?

We could, the reason I didn't is that the version info gets parsed into this struct (which I have made no changes to):

/// Parses a version string into a type that can be used for comparisons.
#[derive(Eq, PartialEq, Debug, Clone)]
pub enum ParsedAppVersion {
    Stable(u32, u32),
    Beta(u32, u32, u32),
    Dev(u32, u32, Option<u32>, String),
}

So the remaining code knows what a "beta" is but has no concept of a an alpha. But we could change the name of the regex variable but it will always only be used to parse a ParsedAppVersion::Beta.

I don't have a strong opinion either way. Should I change the name? Another option is to add an Alpha variant to ParsedAppVersion but then this PR becomes more invloved as then we have to update all places where ParsedAppVersion is used in the Rust code.

[dlon]

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @albin-mullvad)

---

mullvad-types/src/version.rs line 16 at r1 (raw file):

Previously, kl (Kalle Lindström) wrote…

We could, the reason I didn't is that the version info gets parsed into this struct (which I have made no changes to):

/// Parses a version string into a type that can be used for comparisons.
#[derive(Eq, PartialEq, Debug, Clone)]
pub enum ParsedAppVersion {
    Stable(u32, u32),
    Beta(u32, u32, u32),
    Dev(u32, u32, Option<u32>, String),
}

So the remaining code knows what a "beta" is but has no concept of a an alpha. But we could change the name of the regex variable but it will always only be used to parse a ParsedAppVersion::Beta.

I don't have a strong opinion either way. Should I change the name? Another option is to add an Alpha variant to ParsedAppVersion but then this PR becomes more invloved as then we have to update all places where ParsedAppVersion is used in the Rust code.

To be honest, I think the last suggestion sounds like the right one. "Prestable" is probably fine as well, though you'd still have to implement Ord somehow. Pretending they're beta versions is not ideal, IMO.

I don't think that it would affect a lot of code outside of this module if you did either. Seems like we only rely on Ord

[albin-mullvad]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @faern)

---

mullvad-types/src/version.rs line 16 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

To be honest, I think the last suggestion sounds like the right one. "Prestable" is probably fine as well, though you'd still have to implement Ord somehow. Pretending they're beta versions is not ideal, IMO.

I don't think that it would affect a lot of code outside of this module if you did either. Seems like we only rely on Ord

@dlon we discussed it a bit with @faern and decided to try to unify the version structs and logic in mullvad-version and mullvad-types. So this PR will likely be reworked.

[dlon]

Reviewed 4 of 4 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kl)

---

mullvad-version/src/lib.rs line 159 at r3 (raw file):

            }
        } else if self.is_stable() || other.is_stable() {
            // stable > beta|alpha

Is this correct? The beta will be "lower" than any stable version, even if the year is higher, which isn't how it was implemented before. And it assumes that all stable versions are equal.

[dlon]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kl)

---

mullvad-version/src/lib.rs line 159 at r3 (raw file):

Previously, dlon (David Lönnhager) wrote…

Is this correct? The beta will be "lower" than any stable version, even if the year is higher, which isn't how it was implemented before. And it assumes that all stable versions are equal.

I think I'm wrong. Could we add tests (of Ord)?

[albin-mullvad]

Reviewed all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @kl)

---

a discussion (no related file):
Please update the PR title since it's now a bit outdated 🙏

---

-- commits line 2 at r3:
I suggest keeping the subject line on a higher level, e.g: Unify daemon app version types or Unify usage of mullvad-version

Code quote:

Use mullvad-version type, remove ParsedAppVersion

[dlon]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @kl)

[kl]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dlon)

---

mullvad-version/src/lib.rs line 159 at r3 (raw file):

Previously, dlon (David Lönnhager) wrote…

I think I'm wrong. Could we add tests?

I think this should be equivalent to the previous Ord implementation. There are tests in the test\_version\_upgrade\_suggestions function in version_check.rs that indirectly test the Ord implementation. Do you think they are enough or do we want more tests?

[albin-mullvad]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[dlon]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

---

mullvad-version/src/lib.rs line 159 at r3 (raw file):

Previously, kl (Kalle Lindström) wrote…

I think this should be equivalent to the previous Ord implementation. There are tests in the test\_version\_upgrade\_suggestions function in version_check.rs that indirectly test the Ord implementation. Do you think they are enough or do we want more tests?

I would prefer some more direct tests of the Ord implementation, but I think this is good.

[faern]

Reviewed 1 of 4 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @kl)

---

mullvad-version/src/lib.rs line 34 at r4 (raw file):

    pub fn is_stable(&self) -> bool {
        !self.is_alpha() && !self.is_beta() && !self.is_dev()

Slightly nitpicky. But would this not be simpler as self.pre_stable.is_none() && self.dev.is_none()? Shorter to read, and stays constant no matter how many prestable types we hypothetically add.

---

mullvad-version/src/lib.rs line 120 at r4 (raw file):

            .as_str()
            .parse()
            .unwrap();

We should probably return an Err instead of panicking if trying to parse a version string where the year (or the other integers) happens to not be integers. Please also add at least one test for this type of parsing error case 🙏

---

mullvad-version/src/lib.rs line 149 at r4 (raw file):

}

impl Ord for Version {

Please add unit tests for the ordering. This is not trivial code and lends itself well to being unit tested.

[faern]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @kl)

---

mullvad-version/src/lib.rs line 154 at r4 (raw file):

            // dev > stable|beta|alpha
            match (self.is_dev(), other.is_dev()) {
                (true, false) => Ordering::Greater,

Does this not imply that 2025.1-beta1-dev-abc is greater than 2025.1-dev-abc? This should probably not be true since 2025.1-beta1 < 2025.1. Should the dev suffix not only affect the ordering if all other components are equal?

---

mullvad-version/src/lib.rs line 174 at r4 (raw file):

        } else {
            // both are alpha
            Ordering::Equal

In this branch, we must compare the pre_stable numbers. Since 2025.1-alpha2 should be Greater than 2025.1-alpha1 (but less than 2025.1-beta1).

[kl]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @faern)

---

mullvad-version/src/lib.rs line 159 at r3 (raw file):

Previously, dlon (David Lönnhager) wrote…

I would prefer some more direct tests of the Ord implementation, but I think this is good.

Done

---

mullvad-version/src/lib.rs line 34 at r4 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Slightly nitpicky. But would this not be simpler as self.pre_stable.is_none() && self.dev.is_none()? Shorter to read, and stays constant no matter how many prestable types we hypothetically add.

Done

---

mullvad-version/src/lib.rs line 120 at r4 (raw file):

Previously, faern (Linus Färnstrand) wrote…

We should probably return an Err instead of panicking if trying to parse a version string where the year (or the other integers) happens to not be integers. Please also add at least one test for this type of parsing error case 🙏

The regex specifies that the year and incremental version must be integers of a fixed length, so if you give it an invalid year it will return an error (not panic) when matching the regex. I changed the tests for this so that it is more apparent that this function doesn't panic.

---

mullvad-version/src/lib.rs line 149 at r4 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Please add unit tests for the ordering. This is not trivial code and lends itself well to being unit tested.

Done

---

mullvad-version/src/lib.rs line 154 at r4 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Does this not imply that 2025.1-beta1-dev-abc is greater than 2025.1-dev-abc? This should probably not be true since 2025.1-beta1 < 2025.1. Should the dev suffix not only affect the ordering if all other components are equal?

Yes good catch!
I added tests and changed the version struct a little. Now all versions types (stable, beta and alpha) can optionally be dev versions, and dev is only checked at the very end of the comparison chain.

---

mullvad-version/src/lib.rs line 174 at r4 (raw file):

Previously, faern (Linus Färnstrand) wrote…

In this branch, we must compare the pre_stable numbers. Since 2025.1-alpha2 should be Greater than 2025.1-alpha1 (but less than 2025.1-beta1).

Done

[Rawa]

Reviewed 2 of 4 files at r2, 1 of 1 files at r3, 2 of 2 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @faern)

---

mullvad-version/src/main.rs line 78 at r6 (raw file):

    };

    let decade_and_year = version.year % 2000;

nit: This will work until 2100 then it will start returning 100, 101 etc. It should be:

version.year % 100

[Rawa]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @faern and @kl)

[kl]

Reviewable status: 3 of 6 files reviewed, 5 unresolved discussions (waiting on @dlon, @faern, and @Rawa)

---

mullvad-version/src/main.rs line 78 at r6 (raw file):

Previously, Rawa (David Göransson) wrote…

nit: This will work until 2100 then it will start returning 100, 101 etc. It should be:

version.year % 100

Nice catch! 😁 Fixed.

[Rawa]

Reviewable status: 2 of 6 files reviewed, 4 unresolved discussions (waiting on @dlon and @faern)

---

mullvad-version/src/main.rs line 78 at r6 (raw file):

Previously, kl (Kalle Lindström) wrote…

Nice catch! 😁 Fixed.

👍

[Rawa]

Reviewed 2 of 4 files at r7, all commit messages.
Reviewable status: 4 of 6 files reviewed, 4 unresolved discussions (waiting on @faern)

[Rawa]

Reviewed 1 of 4 files at r7.
Reviewable status: 5 of 6 files reviewed, 4 unresolved discussions (waiting on @faern)

[faern]

Reviewed 1 of 4 files at r7, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions

---

mullvad-version/src/lib.rs line 34 at r4 (raw file):

Previously, kl (Kalle Lindström) wrote…

Done

Now you don't check self.dev. 2025.1-dev-abc should not be considered stable, right? Or is a dev-version of a "stable" version actually stable? I doubt that's the logic we want, but we have to check the places where this is used.

---

mullvad-version/src/lib.rs line 120 at r4 (raw file):

Previously, kl (Kalle Lindström) wrote…

The regex specifies that the year and incremental version must be integers of a fixed length, so if you give it an invalid year it will return an error (not panic) when matching the regex. I changed the tests for this so that it is more apparent that this function doesn't panic.

You are correct! My bad. We can safely unwrap here. Great

---

mullvad-version/src/lib.rs line 154 at r4 (raw file):

Previously, kl (Kalle Lindström) wrote…

Yes good catch!
I added tests and changed the version struct a little. Now all versions types (stable, beta and alpha) can optionally be dev versions, and dev is only checked at the very end of the comparison chain.

Awesome!

---

mullvad-version/src/lib.rs line 61 at r7 (raw file):

    /// The following order is used: stable > beta > alpha
    /// If two versions are identical except for one being a dev version
    /// (ending with the suffix -dev-[SHA]), he dev version is considered greater than

s/he/the

---

mullvad-version/src/lib.rs line 64 at r7 (raw file):

    /// the non-dev version, but if both are dev versions they are considered equal
    /// since the dev version SHA is not ordered.
    pub fn is_later_version_than(&self, other: &Self) -> bool {

Can't this return std::cmp::Ordering? We just need it to not implement PartialOrd to not break the rules about PartialOrd + PartialEq, we can still use the ordering type? Doing so will likely make it easier to later use methods such as slice::sort_by and similar to implement sorting.

---

mullvad-version/src/main.rs line 78 at r5 (raw file):

    };

    let decade_and_year = version.year % 2000;

Nitpick. Hard to name variable. But year here is used as if it refers to the last digit in 2025 only, which is a bit weird IMO. How about year_last_two_digits?

---

mullvad-version/src/main.rs line 107 at r5 (raw file):

/// On Windows we currently support the following versions: stable, beta and dev.
fn is_valid_windows_version(version: &Version) -> bool {
    version.is_stable() || version.is_beta()

Why were dev versions removed from this check?

[faern]

Reviewed 1 of 4 files at r2, 1 of 1 files at r6, 2 of 4 files at r7.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @kl)

[kl]

Reviewable status: 2 of 6 files reviewed, 2 unresolved discussions (waiting on @faern and @Rawa)

---

mullvad-version/src/lib.rs line 34 at r4 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Now you don't check self.dev. 2025.1-dev-abc should not be considered stable, right? Or is a dev-version of a "stable" version actually stable? I doubt that's the logic we want, but we have to check the places where this is used.

Yes, I changed the logic to be that way (stable-dev is still stable, just as beta-dev is beta and alpha-dev is alpha). This made implementing the ord logic simpler. I have checked everywhere it is used as part of removing the Ord impl.

---

mullvad-version/src/lib.rs line 61 at r7 (raw file):

Previously, faern (Linus Färnstrand) wrote…

s/he/the

Done

---

mullvad-version/src/lib.rs line 64 at r7 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Can't this return std::cmp::Ordering? We just need it to not implement PartialOrd to not break the rules about PartialOrd + PartialEq, we can still use the ordering type? Doing so will likely make it easier to later use methods such as slice::sort_by and similar to implement sorting.

Done

---

mullvad-version/src/main.rs line 78 at r5 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Nitpick. Hard to name variable. But year here is used as if it refers to the last digit in 2025 only, which is a bit weird IMO. How about year_last_two_digits?

Done

---

mullvad-version/src/main.rs line 107 at r5 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Why were dev versions removed from this check?

Because the logic got changed so that a stable-dev is now considered stable.

[faern]

Reviewable status: 2 of 6 files reviewed, 1 unresolved discussion (waiting on @Rawa)

---

mullvad-version/src/lib.rs line 34 at r4 (raw file):

Previously, kl (Kalle Lindström) wrote…

Yes, I changed the logic to be that way (stable-dev is still stable, just as beta-dev is beta and alpha-dev is alpha). This made implementing the ord logic simpler. I have checked everywhere it is used as part of removing the Ord impl.

But this still does not sound right. If we ignore how easy it is to implement the Ord logic (which should not dictate our public API). When we say "a stable release" we mean an actual release, i.e. 2025.1. A dev version is by definition not "stable", they are dev versions.

[faern]

Reviewed 2 of 4 files at r9.
Reviewable status: 4 of 6 files reviewed, 1 unresolved discussion (waiting on @Rawa)

[faern]

Reviewed 2 of 2 files at r10, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion

---

mullvad-version/src/lib.rs line 48 at r10 (raw file):

    /// dev version (i.e. contains a -dev suffix) and is not a pre-release version (i.e. alpha
    /// or beta). Example: 2025.10
    pub fn is_stable(&self) -> bool {

Nothing calls this except our own unit tests. So even if the implementation looks correct, I'm a bit hesitant to expose an API that is slightly unclear and no one needs.

[kl]

Reviewable status: 5 of 6 files reviewed, all discussions resolved (waiting on @faern)

---

mullvad-version/src/lib.rs line 48 at r10 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Nothing calls this except our own unit tests. So even if the implementation looks correct, I'm a bit hesitant to expose an API that is slightly unclear and no one needs.

Removed

[faern]

Reviewed 1 of 1 files at r11, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved