Fix the Engine version check in GUI #6570

mwu-tow · 2023-05-04T20:13:09Z

Pull Request Description

This PR fixes #6560.

The fix has a few elements:

Bumps the Engine requirement to the latest release, namely 2023.1.1.
Changed the logic of checking whether a given version matches the requirement. Previously, we relied on VersionReq from semver crate which did not behave intuitively when the required version had a prerelease suffix. Now we rely directly on Semantic Versioning rules of precedence.
Code cleanups, including deduplicating 3 copies of the version-checking code, and moving some tests to more sensible places.

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed, the GUI was tested when built using ./run ide build.

…ed version.

kazcw

The previous error message gave instructions to modify a project to use a different engine version, while the new message does not; I assume this is an intentional change.

kazcw · 2023-05-04T20:46:27Z

app/gui/config/src/lib.rs

 // ==============

+/// A wrapper for an error with information that the Engine version does not meet the requirements.


Is this a wrapper for an error or simply an error type?

kazcw · 2023-05-04T20:47:04Z

app/gui/config/src/lib.rs

+    /// The version of the Engine that was found.
+    pub found:    semver::Version,
+}
+


More whitespace.

kazcw · 2023-05-04T20:48:07Z

app/gui/config/src/lib.rs

+) -> Result<(), UnsupportedEngineVersion> {
+    // We don't want to rely on the `semver::VersionReq` semantics here. Unfortunately the
+    // [Semantic Versioning specification](https://semver.org/) does not define the semantics of
+    // of the version requirement operators, so different implementations may behave differently.


Doubled of

kazcw · 2023-05-04T20:48:35Z

app/gui/config/src/lib.rs

+    // [Semantic Versioning specification](https://semver.org/) does not define the semantics of
+    // of the version requirement operators, so different implementations may behave differently.
+    //
+    // The `semver::VersionReq` implementation follows the Cargo's implementation, namely:


~~the~~ Cargo

kazcw · 2023-05-04T20:49:51Z

app/gui/config/src/lib.rs

+    // ```
+    // In particular, in order for any VersionReq to match a pre-release version, the VersionReq
+    // must contain at least one Comparator that has an explicit major, minor, and patch version
+    // identical to the pre-release being matched, and that has a nonempty pre-release component.
+    // ```


Is there a source you can readily link for this?

kazcw · 2023-05-04T20:51:57Z

app/gui/config/src/lib.rs

@@ -64,3 +109,24 @@ pub fn read_args() -> Args {
 lazy_static! {
    pub static ref ARGS: Args = read_args();
 }
+


Missing vertical space.

kazcw · 2023-05-04T20:54:11Z

app/gui/config/src/lib.rs

+        // Sanity check: required version must be supported.
+        assert!(check_engine_version(&engine_version_required()).is_ok());
+    }
+}


What about a test that would fail with semver::VersionReq?

wdanilo · 2023-05-05T03:50:51Z

I don't think this is intentional to not report users with the error that Kaz mentioned above. @mwu-tow ?

wdanilo · 2023-05-05T03:51:35Z

Feel free to merge it after fixing the issues. If there is Kaz review, there is no need for mine :)

mwu-tow · 2023-05-05T14:33:25Z

@wdanilo
I'm not sure. I felt that this warning was not making much sense, especially if the primary use case we target for our clients is a cloud-based project setup. In such cases, the warning gives wrong information, the file is not on the disk, and path on the remote would be totally different (if it was accessible).

…n-check

* develop: (28 commits) Add tests for Date.until, Date.next and Date.previous. (#6606) Improve `Non_Unique_Primary_Key` error, split file format detection into read/write, improve SQLite format detection (#6604) tokenize_to_columns or parse_to_columns results in a single column we shouldn't add the 1 (#6607) Fix node editing race condition (#6594) Add format to the in-memory Column (#6538) Fix dashboard issues (part 2) (#6511) Fix visualisation type selector artifacts rendered after node preview visualisation was closed. (#6575) Revert typescript CI Lint changes (#6602) Fix the Engine version check in GUI (#6570) Show error pop-up when failing to rename a project (#6366) Small changes from Book Club issues (#6533) "at_least_one" flag for tokenize_to_rows (#6539) Benchmark Engine job runs only engine, not Enso benchmarks (#6534) Catch 5813 and avoid crash (#6585) Fix opening links in desktop IDE (#6507) Identify SyntaxError exception and avoid printing a stack trace (#6574) Fix dashboard issues (#6502) Let ChangesetBuilder.invalidated search even container elements (#6548) Fix #5075: stop panning on full-screen visualisation (#6530) Only `Join_Kind.Inner` removes the common-named columns (#6564) ...

…ing-6287 * develop: Fix issues with missing sourcemaps (#6572) Fix asset delete; implement project delete and project rename (#6566) Fix #6377: Change ctrl-r shortcut (#6620) Add tests for Date.until, Date.next and Date.previous. (#6606) Improve `Non_Unique_Primary_Key` error, split file format detection into read/write, improve SQLite format detection (#6604) tokenize_to_columns or parse_to_columns results in a single column we shouldn't add the 1 (#6607) Fix node editing race condition (#6594) Add format to the in-memory Column (#6538) Fix dashboard issues (part 2) (#6511) Fix visualisation type selector artifacts rendered after node preview visualisation was closed. (#6575) Revert typescript CI Lint changes (#6602) Fix the Engine version check in GUI (#6570)

Fix the check if Engine version meets requirements. Bumped the requir…

f5390e3

…ed version.

mwu-tow self-assigned this May 4, 2023

mwu-tow requested review from MichaelMauderer, wdanilo, farmaazon and kazcw as code owners May 4, 2023 20:13

mwu-tow added the CI: No changelog needed Do not require a changelog entry for this PR. label May 4, 2023

kazcw approved these changes May 4, 2023

View reviewed changes

mwu-tow added 4 commits May 5, 2023 16:59

CR

e7078ec

Merge remote-tracking branch 'origin/develop' into wip/mwu/fix-versio…

6b50907

…n-check

Merge remote-tracking branch 'origin/develop' into wip/mwu/fix-versio…

7788a82

…n-check

Merge remote-tracking branch 'origin/develop' into wip/mwu/fix-versio…

59777c7

…n-check

mwu-tow added the CI: Ready to merge This PR is eligible for automatic merge label May 8, 2023

mergify bot merged commit ee8e9e5 into develop May 8, 2023

mergify bot deleted the wip/mwu/fix-version-check branch May 8, 2023 14:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the Engine version check in GUI #6570

Fix the Engine version check in GUI #6570

mwu-tow commented May 4, 2023

kazcw left a comment

kazcw May 4, 2023

kazcw May 4, 2023

kazcw May 4, 2023

kazcw May 4, 2023

kazcw May 4, 2023

kazcw May 4, 2023

kazcw May 4, 2023

wdanilo commented May 5, 2023

wdanilo commented May 5, 2023

mwu-tow commented May 5, 2023

		// ==============

		/// A wrapper for an error with information that the Engine version does not meet the requirements.

Fix the Engine version check in GUI #6570

Fix the Engine version check in GUI #6570

Conversation

mwu-tow commented May 4, 2023

Pull Request Description

Important Notes

Checklist

kazcw left a comment

Choose a reason for hiding this comment

kazcw May 4, 2023

Choose a reason for hiding this comment

kazcw May 4, 2023

Choose a reason for hiding this comment

kazcw May 4, 2023

Choose a reason for hiding this comment

kazcw May 4, 2023

Choose a reason for hiding this comment

kazcw May 4, 2023

Choose a reason for hiding this comment

kazcw May 4, 2023

Choose a reason for hiding this comment

kazcw May 4, 2023

Choose a reason for hiding this comment

wdanilo commented May 5, 2023

wdanilo commented May 5, 2023

mwu-tow commented May 5, 2023