search

OpenModelica / OMPython

A Python interface to OpenModelica communicating via CORBA or ZeroMQ
Other
107 stars 58 forks source link

Changed to use list instead of string for subprocess command. #214

Closed j-emils closed 6 months ago

j-emils commented 6 months ago

Related Issues

Purpose

Cannot execute the simulate method on linux.

Approach

Adapting subprocess input.

CLAassistant commented 6 months ago

CLA assistant check
All committers have signed the CLA.

arun3688 commented 6 months ago

@j-emils Thanks for the fix

arun3688 commented 6 months ago

@j-emils Thanks for the fix

lafrech commented 5 months ago

I came to report this until I realized it was fixed in master branch already. Thanks @j-emils for the fix.

A proper resolution would be to create the list in the first place rather than concatenate strings then split, but oh well.

Also, I believe the list should be split one step further to separate e.g. -r and the file path.

See for instance Popen docs:

    Popen(["/usr/bin/git", "commit", "-m", "Fixes a bug."])

although I can't comment on the consequences of not doing it.

I did it like this as a test but really, it would be more sensible to build the list in the first place.

cmd = sum([r.split("=") for r in cmd.split()], [])

@arun3688 considering the fixes that have been merged since last release, a new release would be greatly appreciated.

Thanks!

casella commented 5 months ago

@j-emils do you mean you'd like to get this fix in OpenModlica 1.23.x?

lafrech commented 5 months ago

In case there is a misunderstanding, I was kindly asking for a OMPython v3.5.2 release to get the latest fixes.

Thanks.

arun3688 commented 5 months ago

@j-emils A new release has been made with v.3.5.2 and also the pip package has been updated with the new release pip install OMPython==3.5.2

lafrech commented 5 months ago

Thank you so much!

j-emils commented 5 months ago

@lafrech, I agree that a more thorough solution would be to create the list from the get go. However, the focus of this fix was to resolve the immediate issue of the code not working. In the future there is a need for refactoring the code in my opinion.

casella commented 5 months ago

@lafrech, I agree that a more thorough solution would be to create the list from the get go. However, the focus of this fix was to resolve the immediate issue of the code not working. In the future there is a need for refactoring the code in my opinion.

You may open another ticket for that, so we don't forget about it.

lafrech commented 5 months ago

@j-emils I totally understand and agree @casella done: #217

Have a nice day, guys.