Change .environment to a function that returns variables instead of strings #1198
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Merged current master
Update the fork with changes from master fork.
Fork update
merge changes
Merging changes to the fork
Merging updates from master fork
Merging changes from master
Merge from master branch
Merge master fork
Updated the way virtual desktop plugin is used.
Merge master fork
Merge master fork
Trailing spaces removal
plata
reviewed
Jan 14, 2020
| if (environmentFunc && environmentFunc instanceof Function) { | ||
| this._environmentFunc= environmentFunc; | ||
| } else { | ||
| throw new Error(tr("The argument of environment() should be a function !")); |
There was a problem hiding this comment.
Why not allow to pass the environment as a string also?
There was a problem hiding this comment.
Because strings aren't directory paths.
There was a problem hiding this comment.
Then I don't understand how it worked before.
There was a problem hiding this comment.
Perhaps it never worked. Because I remember earlier today I did see nvapi workaround being enabled when using old .environment implementation.
There was a problem hiding this comment.
It worked before because the envirnment was assumed to be a string not dependent of any external variable. With this, one can construct the environment with part depedending of wine or user input.
We could also add an option to just pass a plain string instead of a function in the case the string does not depends on wine or user input but it can be done with:
.environment((wine) => {
return '{"DXVK_HUD" : "1" }';
})I think I prefer to use a uniform solution for this.
There was a problem hiding this comment.
I think I prefer to use a uniform solution for this.
I understand this, but I'm not sure I agree in this case.
Adding a second input type is as simple as adding another else if to the .environment(...) implementation. The benefit of this is that you:
- don't get any unused variable messages by Codacy
- make the code more concise
There was a problem hiding this comment.
Well, it is a question of choice. It does not bother me the way I propose it and it is not really that big (in comparaison of .postInstall or .preInstall generally). Always using a lambda function makes the code easier to write I guess (only one way to use and to code).
But if @Zemogiter is motivated ^^.
madoar
requested changes
Jan 14, 2020
| @@ -130,6 +130,8 @@ module.default = class SteamScript extends QuickScript { | |||
| .wizard(setupWizard) | |||
| .prefix(this._name, this._wineDistribution, this._wineArchitecture, this._wineVersion); | |||
|
|
|||
| this._environment = this._environmentFunc(wine); | |||
There was a problem hiding this comment.
Set the _environment variable in .go() from the _environmentFunc, since it cannot be set in .environment() as it could need the wine object. this._environment thus can be used in this_createShortcut()
This PR still needs to add this behaviour for all QuickScript.
There was a problem hiding this comment.
I would prefer if we don't set the variable in object scope but in method scope, i.e.:
const environment = this._environmentFunc(wine);If we do this we will need to forward environment to all methods that use it, i.e. the shortcut creation method
There was a problem hiding this comment.
It is easier in terms of code change that we set it as object change (like it was done before using .environment() method). I guess it is a matter of personal opinion ^^.
There was a problem hiding this comment.
If you set everything in object scope you kind of polute the object. In my opinion an action should be repeatable. This means that if you call .go() twice on the same installer it should lead to the same result. But if you write intermediate results in object scope and maybe even overwrite existing values then this will not be the case anymore.
There was a problem hiding this comment.
You do not overwrite an existing value since _environment was not defined. If you run agian .go() you will overwrite that value, but since it can depend on the wine object it could potentially change, so this is not problem.
Anyway I do not really care how it is done at this point, that is why I prefer the solution that needs the less code modifying/refactor. But you can do how you want.
|
@plata @ImperatorS79 @Zemogiter is there an actual reason why we accept a string instead of a json object in |
|
The string is just directly written to the shortcut file, it is not used during installation (it this the environment used to execute the final program). So it is better to already have the string in my opinion (avoid a JSON.stringify call). We could also add environment variables needed to install the program though. |
I agree that directly using a string has the benefit that we don't need to do a conversion. The issue is just that a string can't be validated by Codacy. If we would return a json object instead, Codacy would have marked #1197 as containing an error. Let me give an example: When using a json object the code in #1197 would look as followed: ...
.environment({
"DXVK_CONFIG_FILE": dxvkConfigFile,
"STAGING_SHARED_MEMORY": 0,
"WINEESYNC": 1
})
...Codacy (or more precise eslint in general) would then detect that the variable I also don't think that the overhead to convert the json object to a string is that large. |
|
So why not. If @Zemogiter is motivated to do the required changes, it can be done. |
|
I agree that we should use an object instead of a string. |
|
@ImperatorS79 what changes you have in mind? Besides changing other quick scripts which you already said. |
|
Any updates on this PR? |
|
I am currently waiting on @ImperatorS79 response to my previous comment to sum up all the things that I have to add to this PR in order for it to be valid. |
|
This is odd. Now the script for |
|
Does this happen only for Space Engineers? |
|
@plata when trying Subnautica script I get the exact same error |
|
So maybe it's not even related to the changes of this pr? |
|
@plata I've removed changes from this PR in my local repo and I've reversed the Terminal log (similiar to the last one but the error does not stop the script from execution): |
|
UPDATE: Script installs Space engineers just fine but even with the old |
This is not valid json |
|
@madoar I know but that's how it looks in |
|
@Zemogiter please note that there's a difference between our API and the way the environment is set e.g. in bash. When calling the |
|
@plata I'm telling you it won't work that way |
Actually it does. Please take a look at the processing code at: scripts/Engines/Wine/Shortcuts/Wine/script.js Lines 183 to 189 in 4d87a17 This code expects the input to be valid json as seen by: scripts/Engines/Wine/Shortcuts/Wine/script.js Line 185 in 4d87a17 |
Open
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
By #1197 we've discovered that when you wanted to set a file path as an variable (like DXVK config file for example) it would register it as a string. This PR is meant to change that.
What works
Subnautica script works after the neccesary changes but wine.log still reports nvapi workaround to be enabled despite env value now correctly asigned and pointing to an existing dxvk.conf file with
dxgi.nvapiHack = Falseinside it.What was not tested
Other QuickScripts, for now
Test
Ready for review
json-alignandeslintrun according to the documentation.