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

Allow version regex to match 4.0.stable.arch_linux #168

Merged
merged 2 commits into from
Mar 14, 2023

Conversation

ttencate
Copy link
Contributor

Also leverage named capture groups and verbose regex mode for slightly more readability.

];

let bad_versions = [
"4.0.unstable.custom_build.e7e9e663b", // 'unstable'
"4.0.3.custom_build.e7e9e663b", // no stability
"3.stable.official.206ba70f4", // no minor
"4.0.alpha.custom_build", // no rev after 'custom_build' (this is allowed for 'official' however)
Copy link
Contributor Author

@ttencate ttencate Mar 11, 2023

Choose a reason for hiding this comment

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

Yes, this removal is on purpose. I don't see why we need to reject this form.

Copy link
Member

@Bromeon Bromeon Mar 12, 2023

Choose a reason for hiding this comment

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

See discussion in #118. Eventually we should probably accept them, but require manual mapping, warning, etc. Maybe the easiest approach for this would be parsing the version into something like:

enum GodotVersion {
    Release{major, minor, patch},
    Git{sha},
    Unknown
}

There's of course a separate discussion to be had whether we should still support alpha/beta/rc now, but at least rc can still occur before minor releases. And since such binaries may still exist, we might want to detect them, to provide more helpful errors. It also separates the version parsing from any semantic implications.

@ttencate ttencate force-pushed the fix/version_regex branch 2 times, most recently from a2e9bce to 4fad500 Compare March 11, 2023 19:49
\.(?P<minor>\d+)
(?:\.(?P<patch>\d+))?
\.(?P<stability>alpha|beta|dev|rc|stable)[0-9]*
(\.[^.]+)+? # mono|official|custom_build|gentoo|arch_linux|...
Copy link
Member

@Bromeon Bromeon Mar 12, 2023

Choose a reason for hiding this comment

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

[^.] might be better constrained as [a-z_], to not capture numbers and special characters.

Why is the whole group repeating? I'd rather make sure we know what the format looks like (and extract more information to be useful when finding compatibility), rather than "consume whatever group comes next"...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See how the version string is built up. Turns out my regex captures the format quite well, but this was entirely by accident :)

A version string like 4.0.stable.foo.bar.baz-1.2.3.mono.mybuild.abcde1234 is actually valid, and can happen if people create their own custom build or link in third-party modules.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks a lot for analyzing this! We should probably document this format somewhere... just where?

I also saw now that you used +?, the lazy operator -- so the Git revision afterwards should always be parsed correctly, if available?

Maybe it makes sense to add some exotic versions to the tests 🙂

Copy link
Contributor Author

@ttencate ttencate Mar 13, 2023

Choose a reason for hiding this comment

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

Based on my analysis, I even made the status (formerly stability) field accept [^.]+. Though we parse it explicitly, we currently don't use it anyway.

Added a comment linking to #118 (comment) so it's documented at least somewhere. Also expanded the comments inside the regex (yay for verbose regex syntax).

godot-codegen/src/godot_version.rs Outdated Show resolved Hide resolved
@ttencate ttencate force-pushed the fix/version_regex branch from 4fad500 to f997699 Compare March 13, 2023 10:53
@Bromeon
Copy link
Member

Bromeon commented Mar 14, 2023

I'll merge this for now, but since this relaxes the version detection which makes compatibility detection harder (see #118), I may soon introduce some restrictions on the version again.

Thanks a lot for this, especially the readable regexes!
bors r+

bors bot added a commit that referenced this pull request Mar 14, 2023
168: Allow version regex to match 4.0.stable.arch_linux r=Bromeon a=ttencate

Also leverage named capture groups and verbose regex mode for slightly more readability.

Co-authored-by: Thomas ten Cate <ttencate@gmail.com>
@bors
Copy link
Contributor

bors bot commented Mar 14, 2023

Build failed:

@ttencate
Copy link
Contributor Author

ttencate commented Mar 14, 2023

Looks like debug builds on MacOS output some additional debug cruft that throws off the parser:

arguments
0: /Users/runner/work/_temp/godot_bin/godot.macos.editor.dev.x86_64
1: --version
Current path: /Users/runner/work/gdext/gdext/godot-core
4.1.dev.custom_build.79454bfd3

I'll see what I can do.

@Bromeon
Copy link
Member

Bromeon commented Mar 14, 2023

Looks like this was accepted under the old regex though? Maybe erroneously? 🤔

It's possible that we should build the Godot binary with a special configuration on macOS.


Btw, found a nice hack to format the regex on GitHub -- use Python highlighting 🙂

        r#"(?x) # ignore whitespace and allow line comments (starting with `#`)
        ^
        (?P<major>\d+)
        \.(?P<minor>\d+)
        # Patch version is omitted if it's zero.
        (?:\.(?P<patch>\d+))?
        # stable|dev|alpha|beta|rc12|... Can be set through an env var when the engine is built.
        \.(?P<status>[^.]+)
        # Capture both module config and build, could be multiple components:
        # mono|official|custom_build|gentoo|arch_linux|...
        # Notice +? for non-greedy match.
        (\.[^.]+)+?
        # Git commit SHA1, currently truncated to 9 chars, but accept the full thing
        (?:\.(?P<custom_rev>[a-f0-9]{9,40}))?
        $
    "#,

@ttencate
Copy link
Contributor Author

Reported godotengine/godot#74906 upstream, and added a workaround.

@ttencate
Copy link
Contributor Author

Looks like this was accepted under the old regex though? Maybe erroneously? 🤔

The old one had no anchors, so we're being a bit stricter here, to make sure we don't parse only a partial version if the rest doesn't conform to our assumptions.

@Bromeon
Copy link
Member

Bromeon commented Mar 14, 2023

bors r+

@bors
Copy link
Contributor

bors bot commented Mar 14, 2023

Build succeeded:

@bors bors bot merged commit f05915f into godot-rust:master Mar 14, 2023
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