Skip to content

deps: Update yaml_cpp deps to fix CRNL and emit issue. #20886

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.

Sign up for GitHub

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

Jump to bottom
Merged
adisuissa merged 3 commits into from
Apr 28, 2022
Merged

deps: Update yaml_cpp deps to fix CRNL and emit issue. #20886

adisuissa merged 3 commits into from
Apr 28, 2022

Conversation

@vehre-x41
Copy link
Contributor

@vehre-x41 vehre-x41 commented Apr 19, 2022 •
edited
Loading

Commit Message: deps: Update yaml_cpp deps to fix CRNL and emit issues.
Additional Description: Update reference to yaml_cpp to make use of latest fixes:

  • fix where on Windows a CRLF is not detected correctly
  • emit of keys having special chars, like &, lead to illegal yaml.
    Signed-off-by: Andre Vehreschild vehre@x41-dsec.de
    Risk Level: Low
    Testing: n/a
    Docs Changes: n/a
    Release Notes: n/a
    Platform Specific Features: Force yaml_cpp to link statically.

@vehre-x41
WIP: Use a patched yaml_cpp.
e2db7ee 
Signed-off-by: Andre Vehreschild <vehre@x41-dsec.de>
@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Apr 19, 2022
@repokitteh-read-only
Copy link

repokitteh-read-only bot commented Apr 19, 2022

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 @wrowe

🐱

Caused by: #20886 was opened by vehre-x41.

see: more, trace.

@vehre-x41 vehre-x41 marked this pull request as draft April 19, 2022 15:22
@vehre-x41 vehre-x41 mentioned this pull request Apr 19, 2022
Closed
keith
keith reviewed Apr 19, 2022
View reviewed changes
Copy link
Member

@keith keith left a comment

Choose a reason for hiding this comment

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

Are there upstream issues / PRs tracking these?

@vehre-x41
Copy link
Contributor Author

vehre-x41 commented Apr 19, 2022 •
edited
Loading

Are there upstream issues / PRs tracking these?

Er, what do you mean? This work is motivated by PR20544 not being the 'right' solution. The issue is in the yaml_cpp library and at least partially fixed there. Unfortunately was my fix (of yaml_cpp) not perfect, so I have to iterate another patch to the yaml_cpp lib. This PR here is just for testing, whether the patch of yaml_cpp now really fixes all issues and builds on all platforms.

@vehre-x41 vehre-x41 mentioned this pull request Apr 19, 2022
Closed
@vehre-x41
Copy link
Contributor Author

vehre-x41 commented Apr 19, 2022

yaml_cpp fixes are also here: yaml_cpp PR1099.

@keith
Copy link
Member

keith commented Apr 19, 2022

Sorry I just meant for the newline one, so jbeder/yaml-cpp#1099, can you add that as a comment in the patch so we know we can remove that once it merges?

@vehre-x41
Copy link
Contributor Author

vehre-x41 commented Apr 20, 2022

/retest

@repokitteh-read-only
Copy link

repokitteh-read-only bot commented Apr 20, 2022

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #20886 (comment) was created by @vehre-x41.

see: more, trace.

@vehre-x41
deps: Use current yaml-cpp to fix CRNL bugs on Windows.
c91ab4a 
Signed-off-by: Andre Vehreschild <vehre@x41-dsec.de>
@vehre-x41
Copy link
Contributor Author

vehre-x41 commented Apr 20, 2022

/donotmerge

This is a temporary state for testing purpose only. DO NOT MERGE!

adisuissa
adisuissa reviewed Apr 25, 2022
View reviewed changes
bazel/repository_locations.bzl Outdated
project_url = "https://github.com/jbeder/yaml-cpp",
version = "db6deedcd301754723065e0bbb1b75927c5b49c7",
sha256 = "387d7f25467312ca59068081f9a25bbab02bb6af32fd3e0aec1bd59163558171",
version = "fa744bbbe22da24c745d991fd8f6f0401af7acf7",
Copy link
Contributor

@adisuissa adisuissa Apr 25, 2022

Choose a reason for hiding this comment

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

@vehre-x41 Thanks for fixing this!
Can this be updated to the latest version 13626af92af7642eba79837842c59602201d52a1? we can then verify that the windows CI is happy.

Copy link
Contributor Author

@vehre-x41 vehre-x41 Apr 26, 2022

Choose a reason for hiding this comment

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

Currently the yaml-cpp project is staling on #1100. I am pursuing this and will update to the latest commit, once #1100 is merged. For now it unfortunately is waiting and nagging the reviewer(s)' of #1100.

/wait

Copy link
Contributor Author

@vehre-x41 vehre-x41 Apr 27, 2022

Choose a reason for hiding this comment

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

yaml-cpp has merged #1100. Updated refs and now just waiting for CI to pass.

@vehre-x41
deps: Update yaml-cpp deps to fix CRNL and emit issue.
3be648f 
Signed-off-by: Andre Vehreschild <vehre@x41-dsec.de>
@vehre-x41 vehre-x41 changed the title WIP: Use a patched yaml_cpp. deps: Update yaml_cpp deps to fix CRNL and emit issue. Apr 27, 2022
@vehre-x41 vehre-x41 marked this pull request as ready for review April 27, 2022 16:02
@moderation
Copy link
Contributor

moderation commented Apr 27, 2022

/lgtm deps

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

moderation commented Apr 27, 2022

/retest

@repokitteh-read-only
Copy link

repokitteh-read-only bot commented Apr 27, 2022

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #20886 (comment) was created by @moderation.

see: more, trace.

adisuissa
adisuissa approved these changes Apr 27, 2022
View reviewed changes
Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@vehre-x41 vehre-x41 mentioned this pull request Apr 28, 2022
Closed
@adisuissa adisuissa mentioned this pull request Apr 28, 2022
Closed
@adisuissa adisuissa merged commit 25c3da2 into envoyproxy:main Apr 28, 2022
ravenblackx pushed a commit to ravenblackx/envoy that referenced this pull request Jun 8, 2022
@vehre-x41 @ravenblackx
deps: Update yaml_cpp deps to fix CRNL and emit issue. (envoyproxy#20886
6222d53 
)

Signed-off-by: Andre Vehreschild <vehre@x41-dsec.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@keith keith keith left review comments

@adisuissa adisuissa adisuissa approved these changes

Assignees

@wrowe wrowe

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@vehre-x41 @keith @moderation @adisuissa @wrowe