fix: update installer to locate KeyShot scripts directory in more cases #123
Conversation
Signed-off-by: Stephen Crowe <[email protected]>
There was a problem hiding this comment.
I think we could simplify this for ourselves and keep the current default of using KEYSHOT/KEYSHOT12 to point to the scripts folder, but add a prompt in the installer to confirm the installation directory and let users pick their own if the default isn't correct? Any opinions on that path? It's called out in our docs that the DCCs should be installed before running the submitter installer so I think we can make the assumption that KeyShot should exist already and not have to handle that case further than maybe printing a warning if we can't find the directory set or specified.
Signed-off-by: Stephen Crowe <[email protected]>
| @@ -66,28 +66,21 @@ | |||
| </conditionRuleList> | |||
| <actionList> | |||
| <!-- KeyShot 2023 env var --> | |||
| <setInstallerVariable name="keyshot_resources_directory" value="${env(KEYSHOT12)}"/> | |||
| <setInstallerVariable name="keyshot_scripts_directory_default" value="${env(KEYSHOT12)}\Scripts"/> | |||
There was a problem hiding this comment.
Should this be keyshot_scripts_directory instead of keyshot_scripts_directory_default?
There was a problem hiding this comment.
Eek, yeah. Rebase issue.
| <allowEmptyValue>0</allowEmptyValue> | ||
| <default>Will be detected from the KEYSHOT or KEYSHOT12 environment variables during installation. If neither exists and the KeyShot component is enabled, this must be input.</default> |
There was a problem hiding this comment.
Why are we removing this? It is used in CLI help text. If we're worried about it no longer matching the existing functionality, can we update to describe that the env var is only used if the directory exists?
There was a problem hiding this comment.
The default property is for the default variable value, not really help text though it might have appeared in CLI help in a convenient location. In some cases, this text was being stored as the value for the scripts directory and appeared inside the directory path text box which looked wrong. I'll move the info about the env variables to the CLI help text.
There was a problem hiding this comment.
With the latest change, the CLI help text looks like this:
There was a problem hiding this comment.
While this is technically intended for default values, it's effectively only used for help text based on the installer definition.
I actually think customers will find it easier to understand with the former version. I find this new version to be confusing because it says that "The default value is detected from the KEYSHOT or KEYSHOT12 environment variables if present." but the default property is missing despite the env vars being present on the machine.
At a minimum, I think we need to clarify that the default value is determined during installation and won't be empty. That's something that isn't clear with an empty default value.
There was a problem hiding this comment.
This default test now appears in the text box when a scripts directory isn't detected, so we can't reuse it in the same way we were before. I'll clarify the help text about the default being determined at runtime.
Signed-off-by: Stephen Crowe <[email protected]>
Signed-off-by: Stephen Crowe <[email protected]>
|
What was the problem/requirement? (What/Why)
The KeyShot installer didn't install the submitter script in some circumstances. Specifically, I had a KeyShot install where the KeyShot resources folder pointed to a directory that did not yet exist.
What was the solution? (How)
What is the impact of this change?
KeyShot installer works in more cases.
How was this change tested?
I ran the installer with KeyShot was installed and without, and in both cases it installed the plugin correctly.
Was this change documented?
Not needed
Is this a breaking change?
No
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.