Update OnlineInstallerScript Default check wine settings #1264

borinquenkid · 2023-01-30T02:19:57Z

Description

Currently OnlineInstallerScript does not work correctly since it defaults to upstream and no version.
Asking for wine settings configures correctly

What works

What was not tested

Test

Operating system (and linux kernel version):
Hardware (GPU/CPU):

Ready for review

Script tested as a regular phoenicis user and working (if you have a problem -> create as draft and ask for help).
json-align and eslint run according to the documentation.
Codacy and travis checked.

plata · 2023-01-31T16:10:13Z

We cannot do it like this. The scripts should use the default or specify it via e.g.

.wineVersion(getLatestStagingVersion)
.wineDistribution("staging")

If the default version (latest stable) is not fetched correctly anymore, we must fix that. See

scripts/Engines/Wine/Engine/Versions/script.js

Line 79 in 7eb3891

*/

Add conditions for MacOSX

borinquenkid

I don't know how to elegantly combine the version logic with the architecture and distribution logic
The only distro-architecture that works on intel mac is cx-x86on64. I imagine that m2 macs would be cx-amd64, so we would need some other flag on the Java side to detect M2

plata

The upstream builds for Darwin do not work?

plata · 2023-02-05T09:57:33Z

Engines/Wine/Engine/Versions/script.js

@@ -122,15 +123,24 @@ module.getAvailableVersions = function (wizard) {


 module.getLatestStableVersion = function (wizard, architecture) {
-    return getLatestVersion(wizard, "upstream-linux-" + architecture, /^\d+\.0(\.\d+)?$/);
+    const os = operatingSystemFetcher.fetchCurrentOperationSystem().getWinePackage()
+    const arch = operatingSystemFetcher.fetchCurrentOperationSystem().getWinePackage() === "darwin" ? "x86on64" : "x86";


Use the architecture parameter. You've already fixed the default _wineArchitecture in QuickScript, so it should work without further changes.

plata · 2023-02-05T09:58:08Z

Engines/Wine/Engine/Versions/script.js

-    return getLatestVersion(wizard, "upstream-linux-" + architecture, /^\d+\.0(\.\d+)?$/);
+    const os = operatingSystemFetcher.fetchCurrentOperationSystem().getWinePackage()
+    const arch = operatingSystemFetcher.fetchCurrentOperationSystem().getWinePackage() === "darwin" ? "x86on64" : "x86";
+    const distribution = operatingSystemFetcher.fetchCurrentOperationSystem().getWinePackage() === "darwin" ?  "cx" : "upstream";


Pass the distribution as additional parameter (like architecture). See

scripts/Engines/Wine/QuickScript/Quick Script/script.js

Line 157 in 7eb3891

this._wineVersion = this._wineVersionFunction(wizard, this._wineArchitecture);

.

borinquenkid · 2023-02-05T23:58:28Z

The upstream builds for Darwin do not work?

MacOS except for really old is all 64 bit, so x86on64 is only cx. If on an AMD64 machine they can switch by hand to amd64 engine.

borinquenkid · 2023-02-05T23:59:15Z

Could not figure out your exact implementation, but I hope this is acceptable

plata · 2023-02-07T15:36:52Z

Engines/Wine/Engine/Versions/script.js

 module.getLatestStableVersion = function (wizard, architecture) {
-    return getLatestVersion(wizard, "upstream-linux-" + architecture, /^\d+\.0(\.\d+)?$/);
+    return getLatestVersion(wizard, `${this._wineDistribution}-${this._winePackage}-${architecture}`, /^\d+\.0(\.\d+)?$/);


The idea was more:

module.getLatestStableVersion = function (wizard, distribution, package, architecture) { return getLatestVersion(wizard, `${distribution}-${package}-${architecture}`, /^\d+\.0(\.\d+)?$/);

Honestly, I don't even understand how the current implementation can work, e.g. this._wineDistribution should not exist here.

I added the field wineDistribution to QuickScript.

borinquenkid · 2023-02-07T20:08:00Z

Engines/Wine/QuickScript/Quick Script/script.js

@@ -4,9 +4,10 @@ const operatingSystemFetcher = Bean("operatingSystemFetcher");

 module.default = class QuickScript {
    constructor() {
+        this._winePackage = operatingSystemFetcher.fetchCurrentOperationSystem().getWinePackage()
+        this._wineArchitecture = this._winePackage === "darwin" ? "x86on64" : "x86";
+        this._wineDistribution = this._winePackage === "darwin" ?  "cx" : "upstream";


Prototyped field wineDistribution

plata · 2023-02-07T21:10:44Z

I understand that the variables exist in QuickScript. However, the "versions" does not derive from it.
And also the scripts are not necessarily QuickScripts, e.g.: https://github.com/PhoenicisOrg/scripts/blob/master/Applications/Internet/Internet%20Explorer%206.0/Online/script.js

plata · 2023-02-14T15:58:00Z

Please check https://phoenicisorg.github.io/scripts/General/tools/. Somehow the formatting got messed up.

borinquenkid · 2023-02-16T05:10:40Z

I ran ESLint

plata · 2023-02-16T15:04:49Z

Engines/Wine/Engine/Versions/script.js

+    return getLatestVersion(wizard, `${this._wineDistribution}-${this._winePackage}-${architecture}`, /^\d+\.0(\.\d+)?$/);
+}
+
+module.getLatestStableVersion = function (wizard, distribution, winePackage, architecture) {


Yes, this is how it should be. Just remove the other getLatestStableVersion and update getLatestDevelopmentVersion and getLatestStagingVersion. Then, ensure that it's called everywhere with the correct parameters.

…entVersion is neeeded

plata · 2023-02-20T09:03:46Z

Applications/Internet/Internet Explorer 6.0/Online/script.js

@@ -31,7 +31,7 @@ new PlainInstaller().withScript(() => {

    const wine = new Wine()
        .wizard(setupWizard)
-        .prefix("InternetExplorer6", "upstream", "x86", getLatestStableVersion(setupWizard, "x86"))
+        .prefix("InternetExplorer6", "upstream", "x86", getLatestStableVersion(setupWizard, null, null, "x86"))


Did you try this? PlainInstaller is not a QuickScript, so I don't see how this._wineDistribution etc. should be set. Generally, I think it would be better to explicitly pass all parameters everywhere (Versions should provide a set of functions and not depend on QuickScript).

plata · 2023-02-20T09:04:06Z

Engines/Wine/QuickScript/Quick Script/script.js


 module.default = class QuickScript {
    constructor() {
+        this._winePackage = operatingSystemFetcher.fetchCurrentOperationSystem().getWinePackage()


Suggested change

this._winePackage = operatingSystemFetcher.fetchCurrentOperationSystem().getWinePackage()

this._winePackage = operatingSystemFetcher.fetchCurrentOperationSystem().getWinePackage();

OnlineInstallerScript now will ask for wine settings

f6056ad

OnlineInstallerScript back to original

fc41ac7

Add conditions for MacOSX

borinquenkid commented Feb 4, 2023

View reviewed changes

plata requested changes Feb 5, 2023

View reviewed changes

Consolidate defintions

f3d75b2

Codacy fix

df6cf9d

plata requested changes Feb 7, 2023

View reviewed changes

borinquenkid commented Feb 7, 2023

View reviewed changes

Address latest PR comments

2689aa2

ESLint

85f7a63

plata reviewed Feb 16, 2023

View reviewed changes

remove getLatestStableVersion one arg. Wondering if getLatestDevelopm…

6cdd730

…entVersion is neeeded

plata requested changes Feb 20, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update OnlineInstallerScript Default check wine settings #1264

Update OnlineInstallerScript Default check wine settings #1264

borinquenkid commented Jan 30, 2023 •

edited

Loading

plata commented Jan 31, 2023

borinquenkid left a comment •

edited

Loading

plata left a comment

plata Feb 5, 2023

plata Feb 5, 2023

borinquenkid commented Feb 5, 2023

borinquenkid commented Feb 5, 2023

plata Feb 7, 2023

borinquenkid Feb 7, 2023

borinquenkid Feb 7, 2023

plata commented Feb 7, 2023

plata commented Feb 14, 2023

borinquenkid commented Feb 16, 2023

plata Feb 16, 2023

plata Feb 20, 2023

plata Feb 20, 2023

	this._winePackage = operatingSystemFetcher.fetchCurrentOperationSystem().getWinePackage()
	this._winePackage = operatingSystemFetcher.fetchCurrentOperationSystem().getWinePackage();

Update OnlineInstallerScript Default check wine settings #1264

Are you sure you want to change the base?

Update OnlineInstallerScript Default check wine settings #1264

Conversation

borinquenkid commented Jan 30, 2023 • edited Loading

Description

What works

What was not tested

Test

Ready for review

plata commented Jan 31, 2023

borinquenkid left a comment • edited Loading

Choose a reason for hiding this comment

plata left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

borinquenkid commented Feb 5, 2023

borinquenkid commented Feb 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plata commented Feb 7, 2023

plata commented Feb 14, 2023

borinquenkid commented Feb 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

borinquenkid commented Jan 30, 2023 •

edited

Loading

borinquenkid left a comment •

edited

Loading