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 Windows cotire build #6508

Merged
merged 26 commits into from
Mar 16, 2020
Merged

Fix Windows cotire build #6508

merged 26 commits into from
Mar 16, 2020

Conversation

HubertBalcerzak
Copy link
Member

This allows building Kratos with cotire on windows

@@ -11,7 +11,7 @@ set CC=cl.exe
set CXX=cl.exe

rem Set variables
set KRATOS_BUILD_TYPE=Release
set KRATOS_BUILD_TYPE=Debug
Copy link
Member

Choose a reason for hiding this comment

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

is it worth updating appveyor?
I would not touch it any more

Copy link
Member

Choose a reason for hiding this comment

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

In any case, lets keep things consistent, its just adding a couple of lines...

@philbucher
Copy link
Member

at the end of the build it says:

  -- Install configuration: "Debug"
  -- Deleting: D:/a/Kratos/Kratos/bin/Debug/KratosMultiphysics
  -- Deleting: D:/a/Kratos/Kratos/bin/Debug/libs

maybe thats why it cant find Kratos when trying to run the tests

@roigcarlo
Copy link
Member

Nono, that's normal, it says that because prior to install we decided to delete the folder. Files get installed after, but at some point we also disable the messages.

The problem is that the env variables defined in the previous stage are not available here.

@philbucher
Copy link
Member

yay it works
well, tests fail :)
maybe too slow in debug?

@philbucher
Copy link
Member

@GuillermoCasas there is a test in the SwimmingDEM app that uses the non-standard python module mpmath. Can you please add a skiptest if this module is not available?
Thx

@roigcarlo
Copy link
Member

Yep, to slow, so... we are a little f*****. Either we increase the runtime for the test, or we compile in Release, and the latter is not an option because then we run out of memory in the machine (cof cof @loumalouomega templates cof cof).

@philbucher
Copy link
Member

Yep, to slow, so... we are a little f*****. Either we increase the runtime for the test, or we compile in Release, and the latter is not an option because then we run out of memory in the machine (cof cof @loumalouomega templates cof cof).

I try now without contact and in release. I mean, the problem with the templates is not precisely new...

@roigcarlo
Copy link
Member

It runs out of memory at core level...

@philbucher
Copy link
Member

oh ok
is this the same as #6284 ?

@roigcarlo
Copy link
Member

Yep...

@philbucher
Copy link
Member

@roigcarlo do you know which files are the problem?
Can we test this somehow?

@philbucher
Copy link
Member

https://docs.microsoft.com/en-us/cpp/build/reference/o-options-optimize-code?view=vs-2019
I am not the expert but I think we could try some things...?

imagen

image

@philbucher
Copy link
Member

now it is able to build and also run the tests in a decent amount of time :D

if fails overall because some tests are failing

@philbucher
Copy link
Member

Tests are failing in
DEM @KratosMultiphysics/dem
SwimmingDEM @KratosMultiphysics/dem @GuillermoCasas
StructuralMechanics @philbucher (my test)

Please take a look and fix those tests such that we can merge this :)

@philbucher
Copy link
Member

@loumalouomega also the contact tests seem to crash in Win

@philbucher
Copy link
Member

@KratosMultiphysics/technical-committee just as an idea, we could for now disable the failing apps, merge this PR and then add the apps back in separate PRs
They are still ensured to compile in Win because they are in appveyor
this way this PR won't hang ...

@philbucher philbucher added this to To do in LEGACY Technical Committee via automation Mar 5, 2020
@roigcarlo
Copy link
Member

Agree

For some reason links against StructuralMechanics???
@philbucher
Copy link
Member

Now it passes :D

@HubertBalcerzak
Copy link
Member Author

@philbucher Is there anything still blocking this PR?

Copy link
Member

@philbucher philbucher left a comment

Choose a reason for hiding this comment

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

To me this can/should go in now, none had any comments and the longer we wait the higher the chances are that some test will break

For the apps I disabled, the are still being compiled in Appveyor so nothing is lost on that end. We should fix the tests and add the apps back here asap though

The night CI that is failing is because there are some tests failing in the nightly suites, this is a known issue, and is being addressed constantly

Thanks for the efforts @HubertBalcerzak @roigcarlo

@roigcarlo
Copy link
Member

@philbucher Seems that windows nightly is mandatory so we cannot merge :S

@philbucher
Copy link
Member

Hm interesting/strange
I will check next week, probably we need to rename sth

@philbucher
Copy link
Member

now it should work once the CI has passed

@philbucher
Copy link
Member

yep now you can merge

@roigcarlo
Copy link
Member

Ok let's do it

@roigcarlo roigcarlo merged commit e440ebf into master Mar 16, 2020
LEGACY Technical Committee automation moved this from To do to Done Mar 16, 2020
@roigcarlo roigcarlo deleted the windows_cotire branch March 16, 2020 10:02
@philbucher
Copy link
Member

🎉 🎉 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants