Skip to content

build: remove -lc++fs from linkopts#7534

Merged
lizan merged 3 commits intoenvoyproxy:masterfrom
lizan:fuzz_fix
Jul 11, 2019
Merged

build: remove -lc++fs from linkopts#7534
lizan merged 3 commits intoenvoyproxy:masterfrom
lizan:fuzz_fix

Conversation

@lizan
Copy link
Copy Markdown
Member

@lizan lizan commented Jul 11, 2019

Signed-off-by: Lizan Zhou lizan@tetrate.io

Description:
Cherry-picking latest commit of #7329 before release. To unbreak fuzz build. Address #7507 that bring real c++fs dependency.

Risk Level: Low
Testing: local fuzz, CI
Docs Changes:
Release Notes:

Signed-off-by: Lizan Zhou <lizan@tetrate.io>
@lizan lizan requested a review from htuch July 11, 2019 05:16
@lizan
Copy link
Copy Markdown
Member Author

lizan commented Jul 11, 2019

Nvm this doesn't work.

@lizan lizan closed this Jul 11, 2019
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
@lizan lizan reopened this Jul 11, 2019
@lizan lizan requested a review from asraa July 11, 2019 07:25
@lizan
Copy link
Copy Markdown
Member Author

lizan commented Jul 11, 2019

@asraa @htuch more context of this PR:

This PR changes #7507 a bit to only use std::__fs when libc++ >= 9.0, so -lc++fs isn't needed in any case. This will unblock #7329 and fuzz build together.

Signed-off-by: Lizan Zhou <lizan@tetrate.io>
@lizan lizan changed the title build: c++fs doesn't have to be explicitly in linkopts build: remove -lc++fs from linkopts Jul 11, 2019
Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

This is fine to unblock oss-fuzz. It's a bit mind boggling how complicated getting a few filesystem libraries is in the C++ ecosystem. To me, this seems a failure in how Clang is handling the rollout of these new libraries, it should be much simpler to just condition on version without also needing to juggle the link complexity.

@lizan lizan merged commit 70ae47e into envoyproxy:master Jul 11, 2019
@lizan
Copy link
Copy Markdown
Member Author

lizan commented Jul 11, 2019

This is fine to unblock oss-fuzz. It's a bit mind boggling how complicated getting a few filesystem libraries is in the C++ ecosystem. To me, this seems a failure in how Clang is handling the rollout of these new libraries, it should be much simpler to just condition on version without also needing to juggle the link complexity.

Agreed, actually #7507 is needlessly complicated and I can clean it up more.

@asraa
Copy link
Copy Markdown
Contributor

asraa commented Jul 11, 2019

Thank you so much for the context as well, @lizan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants