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

Update DXVK (fix OS detection, add new versions) #1039

Merged
merged 39 commits into from
Jan 3, 2020

Conversation

Zemogiter
Copy link
Contributor

@Zemogiter Zemogiter commented Jun 25, 2019

Description

Needs confirmation from someone with a MacOS machine that this script will work as intended.

What works

Everything

What was not tested

Trying to execute a script that installs a game that requires DXVK on a MacOS machine. The script should either skip DXVK installation and go with other verbs (if they are) and install the game or shut down the instalation entirely.

Test

  • Operating system (and linux kernel version): Ubuntu 19.04 x64 5.0.0
  • Hardware (GPU/CPU):
    CPU: i7-7700K
    GPU: GTX1080 ti

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.

Trying to ask the user if they want to continue.
I'm probably wrong but this is what @ImperatorS79 meant with the big if else instruction.
@ImperatorS79
Copy link
Contributor

I meant

if macos
    macos_info
    do you want to go on ? (=> continue)
else if linux
    linux_info
    continue = true;
if continue
    rest of script

@Zemogiter
Copy link
Contributor Author

Zemogiter commented Jun 25, 2019

Made this chart:
https://www.lucidchart.com/documents/view/b1be3e36-45f7-4e55-8ecc-e939e7ac4b5f/0
because I think there is a misunderstanding.

@ImperatorS79
Copy link
Contributor

If yes, you install dxvk, if no, you skip dxvk but still install the app.

@Zemogiter
Copy link
Contributor Author

Thats not what the chart says.

@ImperatorS79
Copy link
Contributor

But that is what I asked previously ^^.

@Zemogiter
Copy link
Contributor Author

Zemogiter commented Jun 25, 2019

But that is what I asked previously ^^.

That is more confusing.

@madoar
Copy link
Collaborator

madoar commented Jun 25, 2019

If you only want to skip the installation of the DXVK verb you can also call return this;.

Copy link
Collaborator

@plata plata left a comment

Choose a reason for hiding this comment

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

Can you check if there are other scripts with the same issue?

Engines/Wine/Verbs/DXVK/script.js Outdated Show resolved Hide resolved
@plata plata changed the title Update DXVK, fixed OS detection Update DXVK (fix OS detection) Jun 26, 2019
Removed a confusing part of the script, thanks for the heads up @plata
@Zemogiter
Copy link
Contributor Author

We still need someone with Mac os to test if this script will work as intended (pressing yes skips this verb but keep the installation of other verbs and the game itself, pressing no quits everything)

Engines/Wine/Verbs/DXVK/script.js Outdated Show resolved Hide resolved
Engines/Wine/Verbs/DXVK/script.js Outdated Show resolved Hide resolved
Using constants instead of variables in beans declaration.
@plata
Copy link
Collaborator

plata commented Jun 29, 2019

@qparis can you test on Mac OS as requested by @Zemogiter?

@Zemogiter Zemogiter changed the title Update DXVK (fix OS detection) Update DXVK (fix OS detection, add new versions) Jul 28, 2019
@Zemogiter
Copy link
Contributor Author

Blocked until PhoenicisOrg/phoenicis#2067 is merged

@madoar madoar requested a review from plata November 29, 2019 18:38
@Zemogiter
Copy link
Contributor Author

Zemogiter commented Dec 4, 2019

There is a problem. The script adds an additional v to the part of url where the filename is located which breaks everything.

[ERROR] org.phoenicis.multithreading.ControlledThreadPoolExecutorService (l.64) - Download of https://github.com/doitsujin/dxvk/releases/download/v1.4.6/dxvk-v1.4.6.tar.gz has failed
	at org.phoenicis.tools.http.Downloader.saveConnectionToStream(Downloader.java:307)
	at org.phoenicis.tools.http.Downloader.get(Downloader.java:260)
	at org.phoenicis.tools.http.Downloader.get(Downloader.java:237)
	at org.phoenicis.tools.http.Downloader.get(Downloader.java:101)
	at org.phoenicis.tools.http.Downloader.get(Downloader.java:81)
	at <js> get(Unnamed:135-144:3538-3839)
	at <js> get(Unnamed:110-117:2786-3026)
	at <js> go(Unnamed:70-76:2648-2915)
	at <js> :anonymous(Unnamed:20:661-679)
	at <js> go(Unnamed:83:3245-3279)
	at com.oracle.truffle.polyglot.ObjectProxyHandler.invoke(HostInteropReflect.java:678)
	at com.sun.proxy.$Proxy68.go(Unknown Source)
	at org.phoenicis.javafx.components.application.skin.ApplicationInformationPanelSkin.lambda$installScript$7(ApplicationInformationPanelSkin.java:237)
	at org.phoenicis.scripts.session.PhoenicisInteractiveScriptSession.eval(PhoenicisInteractiveScriptSession.java:35)
	at org.phoenicis.scripts.interpreter.BackgroundScriptInterpreter.lambda$createInteractiveSession$1(BackgroundScriptInterpreter.java:45)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at java.base/java.lang.Thread.run(Thread.java:834)
Caused by host exception: org.phoenicis.tools.http.DownloadException: Download of https://github.com/doitsujin/dxvk/releases/download/v1.4.6/dxvk-v1.4.6.tar.gz has failed

I suspect its because of getGithubReleases function but I've looked at it and cannot find anything. I've already removed an extra v from DXVK script.

@plata
Copy link
Collaborator

plata commented Dec 4, 2019

The approach is wrong. The script assumes that the download URL is formed in a certain way based on the release version. It would be better to return the complete json and then use tarball_url directly.

@Zemogiter
Copy link
Contributor Author

So how do we need to modify getGithubReleases utility?

@plata
Copy link
Collaborator

plata commented Dec 5, 2019

see #1148

@Zemogiter
Copy link
Contributor Author

Blocked until #1148 is fixed.

@Zemogiter
Copy link
Contributor Author

For some reason the script adds an v between dxvk- and 1.5 while the folder in TMP dosen't have it, thus I'm getting a org.graalvm.polyglot.PolyglotException: Path "/home/jonasz/.Phoenicis/containers//wineprefix//Space Engineers//TMP/dxvk-v1.5/x32" does not exist error.

Fix for the issue mentioned in my last comment.
);
if (operatingSystemFetcher.fetchCurrentOperationSystem().getFullName() !== "Linux")
{
const question = tr("DXVK is currently unsupported on non-Linux operating systems due to MoltenVK implementation being incomplete. Select how do you want to approach this situation.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I still believe that this decision is too complicated for many users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And I still thing otherwise.

@Zemogiter
Copy link
Contributor Author

Now I'm getting this error. Happens before version list shows up:

[ERROR] org.phoenicis.multithreading.ControlledThreadPoolExecutorService (l.64) - TypeError: (intermediate value).withWizard is not a function
	at <js> install(Unnamed:112-113:3950-4029)
	at org.graalvm.polyglot.Value.invokeMember(Value.java:459)
	at org.phoenicis.engines.VerbsManager.lambda$installVerb$0(VerbsManager.java:71)
	at org.phoenicis.scripts.session.PhoenicisInteractiveScriptSession.eval(PhoenicisInteractiveScriptSession.java:35)
	at org.phoenicis.scripts.interpreter.BackgroundScriptInterpreter.lambda$createInteractiveSession$1(BackgroundScriptInterpreter.java:45)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at java.base/java.lang.Thread.run(Thread.java:834)

[WARNING] 
org.graalvm.polyglot.PolyglotException: TypeError: (intermediate value).withWizard is not a function
    at <js>.install (Unnamed:112)
    at org.graalvm.polyglot.Value.invokeMember (Value.java:459)
    at org.phoenicis.engines.VerbsManager.lambda$installVerb$0 (VerbsManager.java:71)
    at org.phoenicis.scripts.session.PhoenicisInteractiveScriptSession.eval (PhoenicisInteractiveScriptSession.java:35)
    at org.phoenicis.scripts.interpreter.BackgroundScriptInterpreter.lambda$createInteractiveSession$1 (BackgroundScriptInterpreter.java:45)
    at java.util.concurrent.ThreadPoolExecutor.runWorker (ThreadPoolExecutor.java:1128)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run (ThreadPoolExecutor.java:628)
    at java.lang.Thread.run (Thread.java:834)

Engines/Wine/Verbs/DXVK/script.js Outdated Show resolved Hide resolved
Engines/Wine/Verbs/DXVK/script.js Outdated Show resolved Hide resolved
@madoar
Copy link
Collaborator

madoar commented Jan 3, 2020

Now the big question: Is the script working? :)

@Zemogiter
Copy link
Contributor Author

Yes 😄

@plata
Copy link
Collaborator

plata commented Jan 3, 2020

@madoar you should merge directly. @Zemogiter doesn't have permission to do so.

@madoar madoar merged commit a37a173 into PhoenicisOrg:master Jan 3, 2020
@Zemogiter Zemogiter deleted the patch-1 branch January 3, 2020 11:26
petermetz pushed a commit to petermetz/scripts that referenced this pull request Jun 7, 2020
* Update script.js
* Replaced version list with getGithubReleases function.
* Code cleanup
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.

5 participants