Skip to content

Conversation

@adisuissa
Copy link
Contributor

Commit Message: deps: yaml-cpp: Updating to latest main version
Additional Description:
Risk Level: Low.
Testing: N/A.
Docs Changes: N/A.
Release Notes: N/A.
Platform Specific Features: N/A.
Fixes #20762

Signed-off-by: Adi Suissa-Peleg [email protected]

@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Apr 11, 2022
@repokitteh-read-only
Copy link

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @moderation

🐱

Caused by: #20765 was opened by adisuissa.

see: more, trace.

@moderation
Copy link
Contributor

Windows CI error looks real

/lgtm deps

@repokitteh-read-only repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Apr 11, 2022
@phlax
Copy link
Member

phlax commented Apr 11, 2022

they dont seem obviously related - other than yaml issues - but there is a failing windows test on the latest dependabot pr to update jinja2 here https://dev.azure.com/cncf/envoy/_build/results?buildId=104760&view=logs&j=4afecb4c-71c7-5b5c-ab99-a70ed4c927ad&t=4cd2fc51-3314-5d69-4df3-f765ae0c08dc&l=1698

@adisuissa
Copy link
Contributor Author

The related failures seem to be due to changes by this PR jbeder/yaml-cpp#1045
That PR was previously reverted, and this introduced a similar fix.
It seems that there was a further issue with PR 1045, described here: jbeder/yaml-cpp#1063
This was later fixed in jbeder/yaml-cpp#1064.

I'll keep digging.

@adisuissa
Copy link
Contributor Author

/wait

Signed-off-by: Adi Suissa-Peleg <[email protected]>
@repokitteh-read-only repokitteh-read-only bot added deps Approval required for changes to Envoy's external dependencies and removed waiting labels Apr 12, 2022
@adisuissa
Copy link
Contributor Author

Any chance of having this change made upstream so we don't have to carry a patch? And please add a TODO(adisuissa): ... on what needs to happen to remove the patch.

/cc @envoyproxy/dependency-shepherds

Yes.
Unfortunately I don't have access to a windows build machine, so using CI for building/testing.
Once we have a good version, I'll send the patch to the yaml-cpp repo.

Signed-off-by: Adi Suissa-Peleg <[email protected]>
Signed-off-by: Adi Suissa-Peleg <[email protected]>
Signed-off-by: Adi Suissa-Peleg <[email protected]>
@moderation
Copy link
Contributor

Related: #20805

@jmarantz
Copy link
Contributor

Adi do you want to merge main to see if that solves the CI issue?

/wait

@adisuissa
Copy link
Contributor Author

This is also being worked on in: #20805
The issue is a broken build of the windows yaml-cpp with Bazel. Still working on reproducing this using a windows VM.

@vehre-x41
Copy link
Contributor

#20805 is now superceeded by #20886 ,which merges main but also had to fix yaml_cpp again. I will try to get the required fixes into yaml_cpp once the build is passing and really everything is fixed...

@adisuissa
Copy link
Contributor Author

Closing in favor of: #20886

@adisuissa adisuissa closed this Apr 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deps Approval required for changes to Envoy's external dependencies waiting

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update yaml-cpp

5 participants