Support Android app's new version code format

[kl]

This PR adds support for the new version code format that we want to start using for the Android app. This code should only affect the Android version parsing and not the other platforms.

The point of the new version code format is to enable use to tag alpha releases and have the build server build them and to have a more flexible format for the future.

The format looks as the following:

/// The format of the code is:                    YYVVXZZZ
///   Last two digits of the year (major)---------^^
///   Incrementing version (minor)------------------^^
///   Build type (0=alpha, 1=beta, 9=stable/dev)------^
///   Build number (000 if stable/dev)-----------------^^^
///
/// # Examples
///
/// Version: 2021.1-alpha1
/// versionCode: 21010001
///
/// Version: 2021.34-beta5
/// versionCode: 21341005
///
/// Version: 2021.34
/// versionCode: 21349000
///
/// Version: 2021.34-dev
/// versionCode: 21349000

Note that this PR is also related to DES-1503 which brings up the inconsistent checking of version strings for the desktop apps. This PR aims to not change that behavior and therefore it actively prevents Windows alpha version strings. For macOS and Linux any version string is allowed.

---

This change is 

[linear]

DROID-485 Improve generated version code

[Pururun]

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

[albin-mullvad]

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

---

mullvad-version/src/lib.rs line 22 at r1 (raw file):

    Beta(String),
    Dev(String),
    Release,

I suggest we call this one Stable rather than Release since we for example often refer to betas by saying "beta release".

Code quote:

Release

[kl]

Reviewable status: 3 of 5 files reviewed, 1 unresolved discussion (waiting on @albin-mullvad and @Pururun)

---

mullvad-version/src/lib.rs line 22 at r1 (raw file):

Previously, albin-mullvad wrote…

I suggest we call this one Stable rather than Release since we for example often refer to betas by saying "beta release".

Done.

[Pururun]

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

[albin-mullvad]

Reviewed 3 of 5 files at r1, 1 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[faern]

Reviewed 2 of 5 files at r1, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @kl)

---

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

        static VERSION_REGEX: LazyLock<Regex> = LazyLock::new(|| {
            Regex::new(
                r"(?x)

You overally improve the readability of the regex here a lot! I love it! If we also just add a comment to this line saying that (?x) enables the free spacing mode that would be awesome

---

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

            (Some(v), _, _) => VersionType::Alpha(v.to_owned()),
            (_, Some(v), _) => VersionType::Beta(v.to_owned()),
            (_, _, Some(v)) => VersionType::Dev(v.to_owned()),

A suggestion could be to replace the _ patterns with None and then add a last catch-all branch for (..) and throw an error? This makes us fail loudly in case the regex/version string is messed up at some point.

---

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

}

#[cfg(test)]

❤️

---

mullvad-version/Cargo.toml line 20 at r3 (raw file):

[dependencies]
regex = "1.11.1"

Was there any newly introduced feature we needed? Upgrading is not bad in any way, I'm just curious why it's needed :)

[kl]

Reviewable status: 4 of 5 files reviewed, 2 unresolved discussions (waiting on @albin-mullvad, @faern, and @Pururun)

---

mullvad-version/Cargo.toml line 20 at r3 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Was there any newly introduced feature we needed? Upgrading is not bad in any way, I'm just curious why it's needed :)

No real reason, I just saw there was a more recent version. Could also revert back to the old version?

---

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

Previously, faern (Linus Färnstrand) wrote…

You overally improve the readability of the regex here a lot! I love it! If we also just add a comment to this line saying that (?x) enables the free spacing mode that would be awesome

Done.

[faern]

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

[faern]

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

---

mullvad-version/Cargo.toml line 20 at r3 (raw file):

Previously, kl (Kalle Lindström) wrote…

No real reason, I just saw there was a more recent version. Could also revert back to the old version?

They are semver compatible, so you don't need to update Cargo.toml. If you want/need a newer regex version then just cargo update -p regex is enough. Bumping version requirements is for when you need to up the lower limit or make a semver incompatible upgrade.

[faern]

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

---

mullvad-version/Cargo.toml line 20 at r3 (raw file):

Previously, faern (Linus Färnstrand) wrote…

They are semver compatible, so you don't need to update Cargo.toml. If you want/need a newer regex version then just cargo update -p regex is enough. Bumping version requirements is for when you need to up the lower limit or make a semver incompatible upgrade.

Great 👍 The upgrade still happens due to the changes to Cargo.lock still being there. But that's fine, let's keep it. Just want to point it out, that it might not be recommended unless there is a specific reason.

[linear]

DES-1503 `mullvad-version` only verifies the Windows version format

[faern]

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

[MarkusPettersson98]

Reviewed 2 of 5 files at r1, 1 of 1 files at r4, 1 of 1 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @kl)

---

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

Previously, faern (Linus Färnstrand) wrote…

❤️

🙌

---

mullvad-version/src/lib.rs line 23 at r6 (raw file):

    Dev(String),
    Stable,
}

This is out of scope, but wouldn't it be cool to distinguish between the version string that Alpha and Beta variants accept vs the Dev variant. The former accept just random version numbers whereas the latter only accept commit-hashes.

Code quote (i):

pub enum VersionType {
    Alpha(String),
    Beta(String),
    Dev(String),
    Stable,
}

Code snippet (ii):

pub enum VersionType {
    Alpha(VersionCode),
    Beta(VersionCode),
    Dev(CommitHash),
    Stable,
}

// Optimally, these would be there own proper types with their own proper parsing logic.
// For that little extra bit of rustiness.
type VersionCode = String; // XX e.g. 01, 02, ..
type CommitHash = String;  // xxyyzz e.g. abc012, 210cba, ..

---

mullvad-version/src/lib.rs line 117 at r6 (raw file):

            .name("dev")
            .map(|m| m.as_str())
            .or(captures.name("no_ver_dev").map(|_| ""));

This feels a bit iffy, why do we event bother extracting no_ver_dev and pass along an empty string instead of returning an error? Maybe there's a good use case, but I feel like that warrants a document by the Dev variant declaration at least.

Code quote:

            .or(captures.name("no_ver_dev").map(|_| ""));

---

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

    if !is_valid_windows_version(&version_type) {
        panic!("Invalid Windows version type: {version_type:?}");
    }

⛏️ I think this might be a teeny tiny bit clearer if expressed as an assert

Code quote (i):

    if !is_valid_windows_version(&version_type) {
        panic!("Invalid Windows version type: {version_type:?}");
    }

Code snippet (ii):

assert!(is_valid_windows_version(&version_type), "Invalid Windows version type: {version_type:?}");

---

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

        VersionType::Beta(_) | VersionType::Dev(_) | VersionType::Stable
    )
}

It would be cool with a comment stating which Windows version types that are considered legal. It seems like Stable, Beta and Dev are considered valid, while Alpha is not. You did such a might fine job explaining the version format in to_android_version_code, and I want more of it 😊

Code quote:

fn is_valid_windows_version(version_type: &VersionType) -> bool {
    matches!(
        version_type,
        VersionType::Beta(_) | VersionType::Dev(_) | VersionType::Stable
    )
}

[faern]

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

[albin-mullvad]

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

---

mullvad-version/src/lib.rs line 72 at r9 (raw file):

            Alpha(version) => write!(f, "-alpha{version}"),
            Beta(version) => write!(f, "-beta{version}"),
            BetaDev { beta, dev } => write!(f, "-beta{beta}-dev-{dev}"),

nit: I suggest we rename this one to commit_hash in order to keep it consistent with the Dev version type

Code quote:

dev

[albin-mullvad]

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

[faern]

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

---

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

use std::str::FromStr;
use std::sync::LazyLock;
use VersionType::*;

Nitpick. But please avoid wildcard imports https://github.com/mullvad/coding-guidelines/blob/main/rust.md#imports-and-modules

[kl]

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

---

mullvad-version/src/lib.rs line 72 at r9 (raw file):

Previously, albin-mullvad wrote…

nit: I suggest we rename this one to commit_hash in order to keep it consistent with the Dev version type

Fixed

---

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

Previously, faern (Linus Färnstrand) wrote…

Nitpick. But please avoid wildcard imports https://github.com/mullvad/coding-guidelines/blob/main/rust.md#imports-and-modules

Fixed

[faern]

Reviewed 2 of 2 files at r11, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @kl)

---

mullvad-version/src/lib.rs line 15 at r11 (raw file):

    pub year: String,
    pub incremental: String,
    pub version_type: Option<VersionType>,

Not a blocker, but I think it would be beneficial with rustdocs on this field explaining what it means. Kind of like what you added for dev below.

---

mullvad-version/src/lib.rs line 68 at r11 (raw file):

        };

        if let Some(dev) = dev {

Maybe name this variable commit_hash for clarity?

[kl]

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

---

mullvad-version/src/lib.rs line 15 at r11 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Not a blocker, but I think it would be beneficial with rustdocs on this field explaining what it means. Kind of like what you added for dev below.

Fixed

---

mullvad-version/src/lib.rs line 68 at r11 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Maybe name this variable commit_hash for clarity?

Fixed

[faern]

NOICE

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

[albin-mullvad]

Reviewed 1 of 2 files at r11, 1 of 1 files at r12, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @kl)

---

mullvad-version/src/lib.rs line 23 at r12 (raw file):

#[derive(Debug, Clone, PartialEq)]
pub enum VersionType {

nit: due to the VersionType no longer representing the Stable type (after other PR feedback) I suggest we rename it to something more suitable, such as PreReleaseType.

---

mullvad-version/src/main.rs line 101 at r12 (raw file):

fn is_valid_windows_version(version_type: &Option<VersionType>) -> bool {
    matches!(version_type, None | Some(VersionType::Beta(_)))

nit: it seems a bit strange to check whether the optional version_type is None part of this function where it's the only argument. IMO it would make more sense to change this so that either:

• This function checks whether a Version is valid, by check its version_type.
• Revert to how it looked before ((version_type: &VersionType)) and let the caller handle the None check.

Code quote:

None 

[albin-mullvad]

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

---

mullvad-version/src/lib.rs line 23 at r12 (raw file):

Previously, albin-mullvad wrote…

nit: due to the VersionType no longer representing the Stable type (after other PR feedback) I suggest we rename it to something more suitable, such as PreReleaseType.

I realize PreReleaseType might not be optimal even though it's quite common to use to refer to alphas and betas. Perhaps PreStableType is more in line with our other related naming.

[kl]

Reviewable status: 3 of 5 files reviewed, 2 unresolved discussions (waiting on @albin-mullvad and @faern)

---

mullvad-version/src/lib.rs line 23 at r12 (raw file):

Previously, albin-mullvad wrote…

I realize PreReleaseType might not be optimal even though it's quite common to use to refer to alphas and betas. Perhaps PreStableType is more in line with our other related naming.

Fixed

---

mullvad-version/src/main.rs line 101 at r12 (raw file):

Previously, albin-mullvad wrote…

nit: it seems a bit strange to check whether the optional version_type is None part of this function where it's the only argument. IMO it would make more sense to change this so that either:

• This function checks whether a Version is valid, by check its version_type.
• Revert to how it looked before ((version_type: &VersionType)) and let the caller handle the None check.

Changed function to taking Version instead

[albin-mullvad]

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

---

mullvad-version/src/lib.rs line 129 at r13 (raw file):

            year,
            incremental,
            pre_stable: version_type,

Is this correct? 🤔

Code quote:

pre_stable: version_type,

[kl]

Reviewable status: 2 of 5 files reviewed, 1 unresolved discussion (waiting on @albin-mullvad, @faern, @MarkusPettersson98, and @Pururun)

---

mullvad-version/src/lib.rs line 129 at r13 (raw file):

Previously, albin-mullvad wrote…

Is this correct? 🤔

Fixed variable name

[albin-mullvad]

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

[linear]

DES-1503 `mullvad-version` only verifies the Windows version format

[faern]

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

[Pururun]

Tested with prepare-release.sh and build-apk.sh and seems to work fine. 👍

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