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

Fix CMake error replace for empty argument #9834

Merged
merged 2 commits into from Feb 16, 2023

Conversation

AnHeuermann
Copy link
Member

Related Issues

I got an error on Windows when compiling with CMake

STRING sub-command REPLACE requires at least four arguments.

because on my system environment variable OMDev is not set.

Purpose

  • Fix CMake error.

Approach

  • Use quotes around last argument for string(REPLACE) in case last argument is empty.

@AnHeuermann AnHeuermann self-assigned this Dec 1, 2022
@AnHeuermann
Copy link
Member Author

@mahge do we have OMDev set on any systems? At most I have OMDEV defined, but currently it's not.
Is

  # Escape the environment variable path
  string(REPLACE "\\" "/" OMDEV_ESCAPED "$ENV{OMDev}")

still needed?

@mahge
Copy link
Contributor

mahge commented Dec 1, 2022

Good question. I think it should be OMDEV too for convention. It works because environment variables are case insensitive on Windows. I didn't notice because it didn't fail.

The whole thing is needed only if you want to run OMEdit and friends from Windows explorer (as opposed to launching from MSYS terminals). In which case we have to copy the MSYS/MinGW dlls to the install directory.

It can probably be replaced by something like you did for FMUs to find the dependency shared libraries and copy them instead of relying on the environment variable. But for that we need to upgrade the CMake version required.

@AnHeuermann
Copy link
Member Author

Ah I see.

[1] 11:13:42 Scripting Notification
We could not find some needed MINGW paths in $OPENMODELICAHOME or $OMDEV. Searched for paths:
D:/workspace/OM/OpenModelica/install_cmake\tools\msys\mingw64\bin [not found]
D:/workspace/OM/OpenModelica/install_cmake\tools\msys\mingw64\lib\gcc\x86_64-w64-mingw32\10.2.0 [not found]

In that case we should give an error from CMake if no OMDev is found?

@AnHeuermann AnHeuermann added the CI/Build MINGW Build pull request with Windows MSYS2 MINGW64 label Dec 2, 2022
@mahge
Copy link
Contributor

mahge commented Dec 2, 2022

Ah I see.

[1] 11:13:42 Scripting Notification
We could not find some needed MINGW paths in $OPENMODELICAHOME or $OMDEV. Searched for paths:
D:/workspace/OM/OpenModelica/install_cmake\tools\msys\mingw64\bin [not found]
D:/workspace/OM/OpenModelica/install_cmake\tools\msys\mingw64\lib\gcc\x86_64-w64-mingw32\10.2.0 [not found]

In that case we should give an error from CMake if no OMDev is found?

@AnHeuermann I think that message is superficial. It can be ignored. You can make OMDEV mandatory so that users don't get confused. In practice you don't technically need to have OMDEV set to build with cmake or use OpenModelica. You need it only so that we know where to copy dlls from to allow running OMEdit and friends from Windows Explorer.

@AnHeuermann
Copy link
Member Author

@adeas31 the OMEdit testsuite is failing on me. I only changed the CMakeList.txt, so as long everything compiles, this shouldn't be able to affect the homotopy tests. Is it just a unreliable test? I had similar issues in PR #9804 and had to restart the Jenkins job a few times.
The test is trying to run each example five times, so I guess I could be just unlucky. I did run the testsuite on my Windows machine and sometimes simulation terminated by an assertion at initialization is found and sometimes it's not.

@mahge Is there already a CMake version of the running make -f Makefile.omdev.mingw omedit-testsuite? It re-compiles everything and clutters my repository with all Makefile build artifacts.

@mahge
Copy link
Contributor

mahge commented Dec 5, 2022

As it happens, I am actually trying to see if I can add it just now 🙂 because @perost needs it as well. I don't know if it will help you in this specific case, but I will try to finish it up quickly.

@adeas31
Copy link
Member

adeas31 commented Dec 6, 2022

The PR is failing at Win/MinGW, AFAIR we don't run the OMEdit tests for Windows.

@adeas31
Copy link
Member

adeas31 commented Dec 6, 2022

The test is trying to run each example five times, so I guess I could be just unlucky. I did run the testsuite on my Windows machine and sometimes simulation terminated by an assertion at initialization is found and sometimes it's not.

There is a timeout of 5 mins instead of waiting for the test result forever. So if the test fail we try 5 times before finally giving up.

@AnHeuermann
Copy link
Member Author

The Jenkins stage Win/MinGW is running common.buildAndRunOMEditTestsuite which builds and runs the OMEdit testsuite. But I had similar issues on a PR without Windows tests, see https://test.openmodelica.org/jenkins/blue/organizations/jenkins/OpenModelica/detail/PR-9804/4/pipeline/1071

@adeas31
Copy link
Member

adeas31 commented Dec 6, 2022

Hmmm.....then we need to come up with a better solution. I will see how to improve this and will make a new PR soon.

@mahge
Copy link
Contributor

mahge commented Dec 6, 2022

The Homotpy test case just hangs until the timeout (5 mins) is over if it fails to compile its generated simulation code for any reason. It should just fail as soon as possible and report the errors instead. @adeas31 Maybe it can be split into separate build and simulate steps?

Another minor thing that can be related is that OMEdit does not report errors it fails to create a given tmp directory for the test cases.

QDir().mkpath(tmpPath);

I don't think that is the cause for the hanging here, but it caused some failures while I was testing and was difficult to figure out what happened.

@adeas31 What should we do if we fail to create the directory? Terminating OMEdit seems drastic but we should at least make sure users do not try and simulate something somehow,

  - Use quotes around last argument for string(REPLACE) in case last
    argument is empty.
  - OMEdit needs OMDEV, so stop with an error if it's not set.
@AnHeuermann AnHeuermann enabled auto-merge (squash) February 16, 2023 14:30
@AnHeuermann AnHeuermann removed the CI/Build MINGW Build pull request with Windows MSYS2 MINGW64 label Feb 16, 2023
@AnHeuermann AnHeuermann merged commit 38c7170 into OpenModelica:master Feb 16, 2023
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.

None yet

3 participants