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

Modelica in different directory management added #122

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

AndreaBartolini
Copy link
Contributor

This pull request checks if the Modelica libraries are reachable from the given libraries path, if not then the Modelica libraries path is get from the system and added to the MODELICAPATH.

@casella casella requested a review from adrpo September 22, 2024 22:53
test.py Outdated
if isWin:
modelicaLibpath = ';' + os.path.normpath(os.path.join(os.environ.get('APPDATA'), '.openmodelica', 'libraries')).replace('\\','/')
else:
modelicaLibpath = ':' + os.path.normpath(os.path.join(os.environ.get('APPDATA'), '.openmodelica', 'libraries')).replace('\\','/')
Copy link

@bilderbuchi bilderbuchi Sep 23, 2024

Choose a reason for hiding this comment

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

Are you sure APPDATA is defined on non-Windows machines?

Same below, if applicable.

Copy link
Contributor Author

@AndreaBartolini AndreaBartolini Sep 23, 2024

Choose a reason for hiding this comment

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

Sorry, maybe I've made a mistake and in the non Win os the APPDATA must be replaced with HOME, as done to set the librariespath variable.

I'll update the PR

Choose a reason for hiding this comment

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

Sorry, you did not observe DRY/duplicated code, so the same thing happens in another place in the PR, hence my remark "Same below": https://github.com/OpenModelica/OpenModelicaLibraryTesting/pull/122/files#diff-f7c50a89e57f8fd5080d06d65e93480d1c23010919e4c42d72563090c7865414R37, which you did not update just now AFAICT.

I'm actually confused how this worked correctly in the first place -- is this tested by CI, or you locally on a Linux and Windows machine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Cristopher, you are right... I forgot that the same code has been added also to the testmodel.py script. I'll update the PR.

Unfortunately I've not a linux machine to test the script locally in this moment (I've to build a VM to do this ... it is in my stack...) so I test on WIN 11 only. For the test under linux I use the CI (I understood that all the PR are tested by the CI under Linux...).

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.

2 participants