feat: Feat/add local launcher #454
Conversation
move close method to SpeosClient class add/adjust close() method in client.py and speos.py
github-actions
bot
added
maintenance
improve close method
|
@echambla @pluAtAnsys @jomadec do we want to add this before the release? |
|
I am a bit hesitate here due to large magick method refactor (maybe?) |
|
@pluAtAnsys can oyu have a look? we also need to think on how to test it? |
add simple test
add simple test
remove test from docker
remove info from local_config.json
remove info from local_config.json
remove info from local_config.json
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #454 +/- ##
==========================================
+ Coverage 86.87% 87.48% +0.60%
==========================================
Files 35 36 +1
Lines 3840 3923 +83
==========================================
+ Hits 3336 3432 +96
+ Misses 504 491 -13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
StefanThoene
changed the title
StefanThoene
requested review from
etiennearnal,
echambla and
jomadec
as code owners
|
@pluAtAnsys could you have a look? it might also make sense to have a look at the documentation for the launch Speos RPC part |
| @@ -108,3 +110,26 @@ def wrapper(*args, **kwargs): | |||
| return method(*args, **kwargs) | |||
|
|
|||
| return wrapper | |||
|
|
|||
|
|
|||
| def error_no_install(install_loc: Union[Path, str], version: Union[int, str]): | |||
There was a problem hiding this comment.
Since this method is only used in launcher.py, I would move it to that file
| install_loc = Path(install_loc) | ||
| exe_notfound = ( | ||
| "Ansys Speos RPC server installation not found at {}." | ||
| " Please define AWP_ROOT{} environment variable" | ||
| ) | ||
| raise FileNotFoundError( | ||
| exe_notfound.format( | ||
| str(Path(install_loc.parent)), | ||
| version, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
| install_loc = Path(install_loc) | |
| exe_notfound = ( | |
| "Ansys Speos RPC server installation not found at {}." | |
| " Please define AWP_ROOT{} environment variable" | |
| ) | |
| raise FileNotFoundError( | |
| exe_notfound.format( | |
| str(Path(install_loc.parent)), | |
| version, | |
| ) | |
| ) | |
| if not install_loc: | |
| install_loc_msg = '' | |
| else: | |
| install_loc_msg = f"at {Path(install_loc).parent}" | |
| install_loc = Path(install_loc) | |
| raise FileNotFoundError( | |
| f"Ansys Speos RPC server installation not found{install_loc_msg}. " | |
| f"Please define AWP_ROOT{version} environment variable" | |
| ) |
Leveraging f-string and better handling of '' as location input.
| ): | ||
| """Initialize the ``SpeosClient`` object.""" | ||
| self._closed = False | ||
| self._remote_instance = remote_instance | ||
| self._command_line = command_line |
There was a problem hiding this comment.
Do you really need to provide this command_line as input ? This may allow an attacker to launch arbitrary executables on the system. When exposing this direct launch mode to untrusted users, it is important to validate that the command is safe or hard-code it in the app.
There was a problem hiding this comment.
but how can i know where the app is installed on the customers PC
There was a problem hiding this comment.
i don't want this variable to be set by anyone else than the launcher ideally
There was a problem hiding this comment.
Couldn't you leverage the app location (once retrieved) to define the command instead ?
There was a problem hiding this comment.
it wouldn't change what would be transmitted really
-> yes i can transfer the install location but that would just remove the executable.
and rename the variable name
| p = subprocess.Popen(command) | ||
| p.wait() |
There was a problem hiding this comment.
| p = subprocess.Popen(command) | |
| p.wait() | |
| subprocess.run(command, check=True) |
| log_level: int = 20, | ||
| speos_rpc_loc: Optional[Union[Path, str]] = None, | ||
| ) -> Speos: | ||
| """Launch Speos RPC server locally. |
There was a problem hiding this comment.
| """Launch Speos RPC server locally. | |
| """Launch Speos RPC server locally. | |
| .. warning:: | |
| Do not execute this function with untrusted input parameters. |
Description
Provide local launcher method
Issue linked
#410
Checklist
feat: add optical property)