-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
[ignition-msgs*] Fix flaky CI tests for ignition-msgs* ports #14464
[ignition-msgs*] Fix flaky CI tests for ignition-msgs* ports #14464
Conversation
We fix the flaky CI tests by not adding anymore the bin and debug/bin directories to the PATH, but rather by making sure that VCPKG_APPLOCAL_DEPS option is correctly propagated.
As I needed to modify the |
Nicole's PR #14399 also tried to fix this issue. |
To clarify, this is a cleanup that should avoid problems of this kind also in the future. |
The osgearth failure is due to this error:
It does not seem to be directly related to this modification. I started #14474 to check if the ports fails also in the current master. |
Unfortunately #14474 worked fine, so either the osqearth failure is non-deterministic, or it is actually related to the change of the PR. Another possibility is that the failure is due to the rebuild of one of osgeart dependencies. |
osgearth has also been sporadically failing; I suspect they have some form of races in their build. As such I don't think we should block merging over that. However, I have a concern with forcing |
command:
Include dirs: And some warnings:
|
That is the reason why it is enabled only for ignition-msgs* ports and not for all the ports. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is the reason why it is enabled only for ignition-msgs* ports and not for all the ports.
Ah, derp, I somehow read the default you set as backwards to what you actually set 🤣
No problem. I think that in general having an opt-in |
I'll merge after CI runs through. |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
isn't that why ADD_BIN_TO_PATH exists? |
Good catch! Yes, I think that would also be sufficient, and will not require the crazy luck of being able to do a full re-build of all the cmake-based ports without any spurious failures, that is apparently really unlikely. I think I can use that one. Given that the path is modified only during the build phase, this should also avoid all the problems of finding the clang as the compiler during the configure phase. |
Waiting for the |
Done in #14730 . |
We fix the flaky CI tests by not adding anymore the bin and debug/bin directories to the
PATH
, but rather by making sure thatVCPKG_APPLOCAL_DEPS
option is correctly propagated.Describe the pull request
What does your PR fix? Fixes the problem mentioned in [ignition] Updates on various pkgs 🤖 #14324 (comment), not sure if there is a corresponding issue.
Which triplets are supported/not supported? Have you updated the CI baseline? The PR should not change the supported triplets.
Does your PR follow the maintainer guide? Yes
cc @BillyONeal