Windows support #58

AndreaBartolini · 2024-02-11T11:10:04Z

This PR partially solves the issue #57, by implement a partial windows integration. This integration has been tested on Win11 pro and it is intended to be used to test modelica libraries locally, on demand or by using a local scheduler.

Not implemented:

FMI integration
Docker integration

Not Tested:

connection to a remote repository
remote URL for reference files

Implementation notes:

the following arguments have been added:

--win (default = False), to activate the windows integration. If --win = False the script runs on linux and all the original features are avalable
--execAllTests (default = False), to force the execution of all tests independently from the last execution date and time
--noSync (default = False), to move files by using python commands instead of rsync, that is not available on windows
--timeout [value] (default = 0), to override the timeout calculated by the script. Override is done if value > 0

arguments parsing has been moved to the beginning of the test.py script because the value of the argument --win shall be available in the entire script

Known issues:

the dygraphs cannot access the .csv files if they are stored in local, this because it performs an XMLHttpRequest to retrieve said files (XMLHttpRequest does not allow the access to local files).
under windows the long paths in the name models are not properly managed, models with long path fail the test

windows integration has been added for basic operations to be executed in local on windows os (tested on Win11 pro). not included: - FMI - Docker not tested - remote URL for reference files

CLAassistant · 2024-02-11T11:10:10Z

All committers have signed the CLA.

bilderbuchi · 2024-02-12T08:20:42Z

--win (default = False), to activate the windows integration. If --win = False the script runs on linux and all the original features are avalable

I'm not sure why you need to introduce a separate command line argument for this? Why don't you query https://docs.python.org/3.11/library/sys.html#sys.platform internally to populate your isWin variable? That would avoid increasing the user-facing complexity.

bilderbuchi · 2024-02-12T08:26:28Z

test.py

+parser = argparse.ArgumentParser(description='OpenModelica library testing tool')
+parser.add_argument('configs', nargs='*')
+parser.add_argument('--branch', default='master')
+parser.add_argument('--fmi', default=False)
+parser.add_argument('--output', default='')
+parser.add_argument('--docker', default='')
+parser.add_argument('--libraries', help="Directory omc will search in to load system libraries/libraries to test.", default=os.path.normpath(os.path.expanduser('~/.openmodelica/libraries/')))
+parser.add_argument('--extraflags', default='')
+parser.add_argument('--extrasimflags', default='')
+parser.add_argument('--ompython_omhome', default='')
+parser.add_argument('--noclean', action="store_true", default=False)
+parser.add_argument('--fmisimulator', default='')
+parser.add_argument('--ulimitvmem', help="Virtual memory limit (in kB) (linux only)", type=int, default=8*1024*1024)
+parser.add_argument('--default', action='append', help="Add a default value for some configuration key, such as --default=ulimitExe=60. The equals sign is mandatory.", default=[])
+parser.add_argument('-j', '--jobs', default=0)
+parser.add_argument('-v', '--verbose', action="store_true", help="Verbose mode.", default=False)
+parser.add_argument('--execAllTests', action="store_true", help="Force all tests to be executed", default=False)
+parser.add_argument('--win', action="store_true", help="Windows mode", default=False)
+parser.add_argument('--noSync', action="store_true", help="Move files using python instead of rsync", default=False)
+parser.add_argument('--timeout', default=0, help="=[value] timeout in seconds for each test, it overrides the timeout calculated by the script")
+
+args = parser.parse_args()


While you are touching this code, maybe it's preferable to use the opportunity to clean up the structure of this script a bit, and put the argument parsing into a separate function that returns the args structure? Thus you reduce the amount of code that just sits at the top level, and proceed towards decoupling the separate bits a little.

shure, I'll implement both the suggestions

bilderbuchi · 2024-02-12T08:27:38Z

test.py

+  if isWin:
+    subprocess.check_output(["python", "testmodel.py", "--help"], stderr=subprocess.STDOUT)
+  else:
+    subprocess.check_output(["./testmodel.py", "--help"], stderr=subprocess.STDOUT)


If you would also invoke with python on Linux, you could avoid this if clause entirely, and it should work just the same?

If I remember right, the point here was also the "./" that in any case should be changed (maybe it's enough to use normpath also here). I'll test this.

AFAIK, you need the ./ prefix if you want to directly run an executable script/binary in the current folder. If you call python testmodel.py, there is no need for the ./, so also no need for normpath I think.

bilderbuchi · 2024-02-12T08:30:40Z

test.py

+      omc_cmd = ["%sbin\\omc" % os.environ.get("OPENMODELICAHOME")]
+    else:
+      omc_cmd = ["%s/bin/omc" % os.environ.get("OPENMODELICAHOME")]


Does OPENMODELICAHOME path trailing slash existence or not differ between Windows and Linux? If so, that should maybe be fixed upstream? @AnHeuermann ?

I think OPENMODELICAHOME isn't set any more when installing OM.
setup-openmodelica will define the variable on Windows, but not on Ubuntu where omc is available via the PATH variable.

So it's probably dead code or maybe the OSMC server still define the variable.

Use os.path.join to unify both cases. It will deal with extra slashes and use the appropriate / or \\.

bilderbuchi · 2024-02-12T08:31:53Z

test.py

-  omc.sendExpression('setModelicaPath("%s")' % librariespath)
-omc_exe=os.path.join(omhome,"bin","omc")
-dygraphs=os.path.join(ompython_omhome or omhome,"share","doc","omc","testmodels","dygraph-combined.js")
+  omc.sendExpression('setModelicaPath("%s")' % librariespath.replace('\\','/'))


os.path.normpath should also convert (back)slashes as appropriate for the platform?

probably yes, I'll check

The issue here is that OMPython needs double escaped \\, so on Windows it would be \\\\....
Ideally OMPython could handle this itself like OMJulia.jl does (https://openmodelica.github.io/OMJulia.jl/dev/api/).

omc does understand Linux style path on Windows, so this is the easy solution.

omc does understand Linux style path on Windows, so this is the easy solution.

That sounds like a straightforward solution. Could maybe also make all those normpath invocations redundant, if I understand correctly.

bilderbuchi · 2024-02-12T08:37:24Z

test.py

+if customTimeout > 0.0:
+  cmd_res=Parallel(n_jobs=n_jobs)(delayed(runScript)(name, customTimeout, data["ulimitMemory"], verbose) for (model,lib,libName,name,data) in tests)
+else:
+  cmd_res=Parallel(n_jobs=n_jobs)(delayed(runScript)(name, 2*data["ulimitOmc"]+data["ulimitExe"]+25, data["ulimitMemory"], verbose) for (model,lib,libName,name,data) in tests)


Instead of duplicating this complicated invocation (and thus making it harder to understand/troubleshoot), why don't you keep it simpler by setting a default and using it appropriately:

if customTimeout == 0: customTimeout = 2*data["ulimitOmc"]+data["ulimitExe"]+25 # the default value cmd_res=Parallel(n_jobs=n_jobs)(delayed(runScript)(name, customTimeout, data["ulimitMemory"], verbose) for (model,lib,libName,name,data) in tests)

That's right, I'll simplify the code

AnHeuermann · 2024-02-12T09:38:27Z

@AndreaBartolini I added tests for Windows so you'll get faster feedback. I'll update the tests when you are done, so this PR doesn't complain about missing tests.

AndreaBartolini · 2024-02-12T09:38:29Z

The PR fails the last CI, and the failure seems to be caused by the following signal handler definition (see the figure below):

This definition was already present in the original script, I just skip it in the execution under windows because windows doesn't allow this kind of definition.

Sincerly I don't undestand the reason of this failure. From the linux point of view this part of the script should be the same of the original.

AndreaBartolini · 2024-02-12T09:43:09Z

@AndreaBartolini I added tests for Windows so you'll get faster feedback. I'll update the tests when you are done, so this PR doesn't complain about missing tests.

Thanks...

test.py

AnHeuermann · 2024-02-28T16:34:27Z

The Linux version is running again.

And I got most of the script running on my Windows machine using Conda and installed OpenModelica OpenModelica v1.23.0-dev-333-g4f26c776af (64-bit).

I think the compilation is broken because of some remaining MINGW/UCRT stuff. Basically the compile.bat is not called with the UCRT case.

AnHeuermann · 2024-02-29T10:58:17Z

We are very close:

Windows OpenModelica v1.21.0

Windows OpenModelica nightly

I'll update the example PNlib to a version where all models are verifying and will update the HTML check.
And then I'll allow the Windows test to fail for the nightly version. I think we need to fix this upstream and I have a rough idea what's broken in the compile.bat script.

AnHeuermann · 2024-02-29T15:38:31Z

@AndreaBartolini From my point of view this is good to go as soon as all three required tests are passing.
Can I squash and merge this PR?

We'll fix the remaining issue in OpenModelica or if it's an issue in this repo in a later commit.

testmodel.py

bilderbuchi · 2024-03-01T10:40:22Z

@AnHeuermann
So many brackets

You know you're allowed to use f-strings and linebreaks and implcit string concatenation in Python, right? ;-) :-P

In this case e.g. (untested)

dockerarg = ("--docker %s --dockerExtraArgs '%s'" % (docker, " ".join(dockerExtraArgs))) if docker else ""
res_cmd = runCommand(
    (f"python testmodel.py --win --msysEnvironment={msysEnvironment}" 
     f" --libraries={librariespath} {dockerarg} "
     f"--ompython_omhome={ompython_omhome} "
     f"{c}.conf.json > files/{c}.cmdout 2>&1"),
    prefix=c,
    timeout=timeout,
)

or however you want to structure this.

AnHeuermann · 2024-03-01T10:57:43Z

Yeah, but I didn't want to change the scripts in this PR (when possible). But I agree, they are the better way to do this 😄

Ideally I would like to transform these scripts into a nice Python module that can be installed via pip or similar. With versioning, testing and stuff like that. This would simplify https://github.com/OpenModelica/openmodelica-library-testing-action a lot.
Maybe a good task for ChatGPT and someone with a bit more Python experience.

AndreaBartolini · 2024-03-02T11:01:11Z

Hi, today I updated my local repo:

but running the script under my Win11 pro I got some strange permisisons error (attached the entire output from my terminal).
Maybe some process is locking the file?

OpenModelica v1.23.0-dev-353-g2e82712898 (64-bit)
Win11 pro 64 bit
output.txt

AndreaBartolini · 2024-03-03T14:12:04Z

Tested again after PC restart, scripts work fine.
Maybe some ghost process was locking the file...

AndreaBartolini added 2 commits February 11, 2024 11:10

partial windows integration

222bee9

windows integration has been added for basic operations to be executed in local on windows os (tested on Win11 pro). not included: - FMI - Docker not tested - remote URL for reference files

help added to some arguments

42752b8

AndreaBartolini mentioned this pull request Feb 11, 2024

Add Windows support and CI #57

Closed

bilderbuchi reviewed Feb 12, 2024

View reviewed changes

AnHeuermann added 2 commits February 12, 2024 10:32

Adding windows-latest to CI

5a503e7

Don't fail fast (for testing)

ef62be7

AnHeuermann reviewed Feb 12, 2024

View reviewed changes

test.py Outdated Show resolved Hide resolved

AnHeuermann added 3 commits February 28, 2024 16:44

Fixing reference files and signal

69e6700

Fixing omc not found

2d44655

Setting libraries for Windows

67ab9ef

Better default value for libraries

af91007

Updating sanity check

a96e92d

AnHeuermann assigned AndreaBartolini and AnHeuermann Feb 29, 2024

AnHeuermann added the enhancement label Feb 29, 2024

Changing leftover 2.2 version to 3

c450a68

AnHeuermann reviewed Feb 29, 2024

View reviewed changes

testmodel.py Outdated Show resolved Hide resolved

Specify MSYS environment on Windows

89af088

AnHeuermann added 3 commits February 29, 2024 17:36

Fixing workflow

62e904d

So many brackets

66e76d5

Last commit for the day

a75bcbf

AnHeuermann merged commit 045cb46 into OpenModelica:master Mar 1, 2024
4 of 5 checks passed

AnHeuermann linked an issue Mar 1, 2024 that may be closed by this pull request

Add Windows support and CI #57

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows support #58

Windows support #58

AndreaBartolini commented Feb 11, 2024 •

edited

Loading

CLAassistant commented Feb 11, 2024 •

edited

Loading

bilderbuchi commented Feb 12, 2024 •

edited

Loading

bilderbuchi Feb 12, 2024

AndreaBartolini Feb 12, 2024

bilderbuchi Feb 12, 2024

AndreaBartolini Feb 12, 2024

bilderbuchi Feb 12, 2024

bilderbuchi Feb 12, 2024

AnHeuermann Feb 12, 2024

AnHeuermann Feb 12, 2024

bilderbuchi Feb 12, 2024

AndreaBartolini Feb 12, 2024

AnHeuermann Feb 12, 2024

bilderbuchi Feb 12, 2024

bilderbuchi Feb 12, 2024 •

edited

Loading

AndreaBartolini Feb 12, 2024

AnHeuermann commented Feb 12, 2024

AndreaBartolini commented Feb 12, 2024 •

edited

Loading

AndreaBartolini commented Feb 12, 2024

AnHeuermann commented Feb 28, 2024

AnHeuermann commented Feb 29, 2024

AnHeuermann commented Feb 29, 2024

bilderbuchi commented Mar 1, 2024 •

edited

Loading

AnHeuermann commented Mar 1, 2024

AndreaBartolini commented Mar 2, 2024 •

edited

Loading

AndreaBartolini commented Mar 3, 2024

Windows support #58

Windows support #58

Conversation

AndreaBartolini commented Feb 11, 2024 • edited Loading

CLAassistant commented Feb 11, 2024 • edited Loading

bilderbuchi commented Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bilderbuchi Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnHeuermann commented Feb 12, 2024

AndreaBartolini commented Feb 12, 2024 • edited Loading

AndreaBartolini commented Feb 12, 2024

AnHeuermann commented Feb 28, 2024

AnHeuermann commented Feb 29, 2024

AnHeuermann commented Feb 29, 2024

bilderbuchi commented Mar 1, 2024 • edited Loading

AnHeuermann commented Mar 1, 2024

AndreaBartolini commented Mar 2, 2024 • edited Loading

AndreaBartolini commented Mar 3, 2024

AndreaBartolini commented Feb 11, 2024 •

edited

Loading

CLAassistant commented Feb 11, 2024 •

edited

Loading

bilderbuchi commented Feb 12, 2024 •

edited

Loading

bilderbuchi Feb 12, 2024 •

edited

Loading

AndreaBartolini commented Feb 12, 2024 •

edited

Loading

bilderbuchi commented Mar 1, 2024 •

edited

Loading

AndreaBartolini commented Mar 2, 2024 •

edited

Loading