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.

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

build: envoy picks up system libidn2-dev package and fails to link #14506

Closed
kyessenov opened this issue Dec 22, 2020 · 5 comments · Fixed by #14601
Closed

build: envoy picks up system libidn2-dev package and fails to link #14506

kyessenov opened this issue Dec 22, 2020 · 5 comments · Fixed by #14601
Assignees
@wrowe @htuch
Labels
area/build area/dns deps Approval required for changes to Envoy's external dependencies
Milestone
1.17.0

Comments

@kyessenov
Copy link
Contributor

libidn2-dev debian package is detected by curl build but should be overridden for reproducible builds.
@kyessenov kyessenov added bug triage Issue requires triage labels Dec 22, 2020
@kyessenov kyessenov mentioned this issue Dec 22, 2020
Merged
@wrowe wrowe added deps Approval required for changes to Envoy's external dependencies area/build area/dns and removed bug triage Issue requires triage labels Dec 22, 2020
@wrowe
Copy link
Contributor

wrowe commented Dec 22, 2020

This is fubar upstream. They've failed to add a undesired feature guard in their CMakeLists.txt here

# Check for idn
check_library_exists_concat("idn2" idn2_lookup_ul HAVE_LIBIDN2)

I think we must reject/revert 7.74, until upstream adds the appropriate CURL_DISABLE_IDN2 toggle. @htuch your thoughts?

@wrowe wrowe added this to the 1.17.0 milestone Dec 22, 2020
@wrowe wrowe assigned wrowe and htuch Dec 22, 2020
@wrowe
Copy link
Contributor

wrowe commented Dec 22, 2020

The alternative is to patch for the use of 7.74, but I'm hoping we could see an upstream 7.74.1 before our 1.17.0 package.

@wrowe wrowe mentioned this issue Dec 22, 2020
Closed
@wrowe
Copy link
Contributor

wrowe commented Dec 23, 2020

The proposed patch curl/curl#6362 would have us introduce USE_LIBIDN2: "off" to the cache_entries of envoy_cmake_external in

envoy/bazel/foreign_cc/BUILD

Line 105 in b4ca626

cache_entries = {

Just waiting to see that this is the correct field name and patch is accepted before injecting it into our baze/foreign_cc/curl.patch, but this workaround seems fine.

@htuch
Copy link
Member

htuch commented Jan 5, 2021

Ideally let's get this option added upstream, so we only need to modify the BUILD.

@wrowe wrowe mentioned this issue Jan 8, 2021
Merged
@wrowe
Copy link
Contributor

wrowe commented Jan 8, 2021 •
edited
Loading

Ideally let's get this option added upstream, so we only need to modify the BUILD.

Agreed @htuch - I've run the draft proposed curl#6362 through our schema in our PR
#14601 for the 1.17.0 milestone, and this looks
correct to me, but it's blocked on a dep-maintainter's review. Would you mind taking that
review please? If we pass it, I'll relay our success to the curl PR.

@kyessenov hopefully you observe the same and green-light the upstream fix?

htuch pushed a commit that referenced this issue Jan 8, 2021
@wrowe
Resolve 14506, avoid libidn2 for our curl dependency (#14601)
4302eb1 
- Reviews the proposal of @jay to resolve libidn2 feature election curl/curl#6362
- Uses -U2 for the patch, to ensure placement but not useless collisions
- Moves extended text of the problems addressed to the patch

This additional patch to dodge idn2 is draft, but hopefully resembles the final patch
to let us pass a simple toggle to avoid libidn2 for machines deployed without
libidn2 libs. In any case, any dynamic additional ld requirement goes against the
static build model, and libidn2 would have to be added to the envoy build.
As we expect to deprecate curl, this would be movement in the wrong direction.

Risk Level: low
Testing: local
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: Linux ldd bindings
Fixes: #14506

Signed-off-by: William A Rowe Jr <wrowe@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@wrowe wrowe

@htuch htuch

Labels
area/build area/dns deps Approval required for changes to Envoy's external dependencies
Projects
None yet
Milestone
1.17.0
Development

Successfully merging a pull request may close this issue.

Resolve 14506, avoid libidn2 for our curl dependency wrowe/envoy
3 participants
@wrowe @kyessenov @htuch