Skip to content

Correct Windows build defects noted with recent version updates#10469

Merged
mattklein123 merged 3 commits intoenvoyproxy:masterfrom
greenhouse-org:review-externals-libcmt
Mar 23, 2020
Merged

Correct Windows build defects noted with recent version updates#10469
mattklein123 merged 3 commits intoenvoyproxy:masterfrom
greenhouse-org:review-externals-libcmt

Conversation

@wrowe
Copy link
Copy Markdown
Contributor

@wrowe wrowe commented Mar 20, 2020

Description:
Picks up from #10433
windows build regression...

  • Pick up curl-7_69_1
  • Patch curl to revert change to cmake_minimum_required
    • latest version of curl added a maximum version of cmake 3.16
  • Pick up cares-1_16_0
  • Toggle crosstool build to avoid possibly injecting wrongly assumed
    flags (/MD or -MD dynamic linkage override of our /MT static build)
  • Note that some issues were pre-existing but not previously triggered

Risk Level: Low
Testing: Local on MSVC
Docs Changes: n/a
Release Notes: n/a

@sunjayBhatia @wrowe
Correct Windows build defects noted with recent version updates
06486aa 
- Pick up curl-7_69_1
- Patch curl to revert change to cmake_minimum_required
  - latest version of curl added a maximum version of cmake 3.16
- Pick up cares-1_16_0
- Toggle crosstool build to avoid possibly injecting wrongly assumed
  flags (/MD or -MD dynamic linkage override of our /MT static build)

Signed-off-by: Sunjay Bhatia <[email protected]>
Signed-off-by: William A Rowe Jr <[email protected]>
Co-authored-by: Sunjay Bhatia <[email protected]>
Co-authored-by: William A Rowe Jr <[email protected]>
@wrowe wrowe mentioned this pull request Mar 20, 2020
Closed
sunjayBhatia and others added 2 commits March 20, 2020 16:00
@sunjayBhatia @wrowe
Correct OS/X and Format failures
5a6d47e 
Drop the changes specifing crosstool compilation, as this broke
OS/X (and introduced the format failure)

Signed-off-by: Sunjay Bhatia <[email protected]>
Signed-off-by: William A Rowe Jr <[email protected]>
Co-authored-by: Sunjay Bhatia <[email protected]>
Co-authored-by: William A Rowe Jr <[email protected]>
@wrowe @sunjayBhatia
Revert the bump of c-ares, the new release segfaults
35612d5 
The 1.16.0 release is unusable by Envoy, according to the
linux asan and tsan CI builds.

Co-authored-by: Sunjay Bhatia <[email protected]>
Co-authored-by: William A Rowe Jr <[email protected]>
Signed-off-by: Sunjay Bhatia <[email protected]>
Signed-off-by: William A Rowe Jr <[email protected]>
@mattklein123
Copy link
Copy Markdown
Member

mattklein123 commented Mar 20, 2020

Assigning to @junr03 to potentially help sort out the c-ares issues.

@junr03
Copy link
Copy Markdown
Member

junr03 commented Mar 20, 2020 •
edited
Loading

From the ASAN failure in https://dev.azure.com/cncf/4684fb3d-0389-4e0b-8251-221942316e06/_apis/build/builds/35151/logs/68

It looks to me that this is a bug in c-ares:

  else if (status == ARES_EDESTRUCTION)
    {
      end_hquery(hquery, status);
    }

  if (!hquery->remaining)

end_query frees hquery but the code then proceeds to keep using hquery. Which is exactly what the ASAN run is catching.

@sunjayBhatia
Copy link
Copy Markdown
Member

sunjayBhatia commented Mar 23, 2020

Some failing macos tests from the previous bump PR (see: this build) and this one (see: this build)

test/common/network/connection_impl_test.cc fails in the former and IpVersions/ActiveQuicListenerTest.ReceiveFullQuicCHLO/IPv6 in the latter 🤔

@wrowe wrowe mentioned this pull request Mar 23, 2020
Merged
@wrowe
Copy link
Copy Markdown
Contributor Author

wrowe commented Mar 23, 2020

@mattklein123 this patch makes no changes to affecting quiche or the ActiveQuicListenerTest, as OS/X reports. We should move ahead with this patch to unblock the curl update.

mattklein123
mattklein123 approved these changes Mar 23, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks!

@mattklein123 mattklein123 merged commit 0b48007 into envoyproxy:master Mar 23, 2020
@moderation
Copy link
Copy Markdown
Contributor

moderation commented Mar 23, 2020 •
edited
Loading

This PR didn't end up updating c-ares from the looks of it @wrowe :

    com_github_c_ares_c_ares = dict(
        sha256 = "de058ad7c128156e2db6dc98b8a359924d6f210a1b99dd36ba15c8f839a83a89",
        strip_prefix = "c-ares-1.16.0",
        urls = ["https://github.com/c-ares/c-ares/releases/download/cares-1_16_0/c-ares-1.16.0.tar.gz"],
    ),

and change to c-ares patch as per #10433 (comment)

@wrowe
Copy link
Copy Markdown
Contributor Author

wrowe commented Mar 24, 2020

You are correct. We began by updating c-ares in the same lot as curl. However, as noted above, c-ares 1.16 introduced new regressions that must be addressed before it is ready for master.

@wrowe
Copy link
Copy Markdown
Contributor Author

wrowe commented Mar 24, 2020

(From the comments above, it appears c-ares regressions are upstream code defects, not envoy's use of the library.)

@wrowe wrowe deleted the review-externals-libcmt branch March 24, 2020 13:49
@sunjayBhatia sunjayBhatia mentioned this pull request Apr 24, 2020
Closed
@junr03 junr03 mentioned this pull request May 11, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattklein123 mattklein123 mattklein123 approved these changes

Assignees

@junr03 junr03

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@wrowe @mattklein123 @junr03 @sunjayBhatia @moderation