Skip to content

build: simplify cc_configure and static link for libc++ #7329

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
merged 10 commits into from
Jul 11, 2019
Merged

build: simplify cc_configure and static link for libc++ #7329

merged 10 commits into from
Jul 11, 2019

Conversation

lizan
Copy link
Member

@lizan lizan commented Jun 19, 2019

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

Description:
Take a similar approach of bazelbuild/bazel@ab9c1f5, which use -l:libstdc++.a to statically link libstdc++. This makes us closer to remove our own cc_wrapper and cc_configure in the future. Also it will allow us do static link with libc++.

Risk Level: Med
Testing: local, CI
Docs Changes:
Release Notes:

@lizan
build: simplify cc_configure
56d5bd0 
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
@lizan lizan requested review from PiotrSikora and htuch June 19, 2019 21:48
@lizan lizan changed the title build: simplify cc_configure WIP: build: simplify cc_configure Jun 19, 2019
@lizan
fix
76bbfa9 
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
@mattklein123
Copy link
Member

cc @keith @jmillikin-stripe

jmillikin-stripe
jmillikin-stripe reviewed Jun 20, 2019
View reviewed changes
bazel/cc_configure.bzl
@@ -108,7 +59,6 @@ cc_autoconf = repository_rule(
"USE_CLANG_CL",
"CC",
"CFLAGS",
"CXX",
Copy link
Contributor

Choose a reason for hiding this comment

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

When building with GCC, does the final link happen with gcc or g++?

Copy link
Member Author

Choose a reason for hiding this comment

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

gcc

lizan added 4 commits June 20, 2019 04:51
@lizan
fix asan and more
f4c1fc2 
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
@lizan
fix
9f0eec5 
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
@lizan
tsan fix
900b808 
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
@lizan
retrofit bazel 0.26
dae720b 
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
@lizan lizan changed the title WIP: build: simplify cc_configure build: simplify cc_configure and static link for libc++ Jun 20, 2019
@lizan
Copy link
Member Author

lizan commented Jun 20, 2019

@jmillikin-stripe @PiotrSikora this is ready now and passes all test, PTAL.

PiotrSikora
PiotrSikora reviewed Jun 22, 2019
View reviewed changes
bazel/foreign_cc/BUILD
@@ -118,6 +118,7 @@ envoy_cmake_external(
"ENABLE_LIB_ONLY": "on",
"CMAKE_BUILD_TYPE": "RelWithDebInfo",
"CMAKE_INSTALL_LIBDIR": "lib",
"CMAKE_CXX_COMPILER_FORCED": "on",
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we set CMAKE_C_COMPILER_FORCED set as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't add it because it wasn't needed, but fine to add as well. no strong opinion on this.

PiotrSikora
PiotrSikora previously approved these changes Jun 22, 2019
View reviewed changes
htuch
htuch reviewed Jun 24, 2019
View reviewed changes
Copy link
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.

Looks good, can you verify this doesn't break oss-fuzz? This is a kind of scary change that is worth verifying. Instructions to test are at https://github.com/envoyproxy/envoy/tree/master/test/fuzz#running-fuzz-tests-locally. CC @asraa for fuzz build visibility.

@stale
Copy link

stale bot commented Jul 1, 2019

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 1, 2019
@stale
Copy link

stale bot commented Jul 8, 2019

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot closed this Jul 8, 2019
@lizan lizan reopened this Jul 8, 2019
@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jul 8, 2019
@lizan
Copy link
Member Author

lizan commented Jul 8, 2019

back from vacation, will check fuzz locally.

@lizan
Copy link
Member Author

lizan commented Jul 10, 2019

@htuch should be good along with google/oss-fuzz#2586

htuch
htuch approved these changes Jul 10, 2019
View reviewed changes
Copy link
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.

Thanks!

@lizan
Copy link
Member Author

lizan commented Jul 10, 2019

@htuch google/oss-fuzz#2586 is merged, but let's hold merging this until 1.11.0.

@htuch
Copy link
Member

htuch commented Jul 10, 2019

@lizan I thought this would happen at the same time as the oss-fuzz merge, I think that PR will break Envoy build without this one. CC @asraa

@lizan
Copy link
Member Author

lizan commented Jul 11, 2019

@htuch I'm fine either way, let me see if that PR break fuzz with current master.

@lizan
Copy link
Member Author

lizan commented Jul 11, 2019

@htuch ah yeah the oss-fuzz breaks, I'm fine merge this, up to you.

@lizan lizan mentioned this pull request Jul 11, 2019
Merged
lizan added a commit that referenced this pull request Jul 11, 2019
@lizan
build: remove -lc++fs from linkopts (#7534)
70ae47e 
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 merged commit 00a6251 into envoyproxy:master Jul 11, 2019
@lizan lizan deleted the static_link_revisit branch July 11, 2019 19:45
@moderation moderation mentioned this pull request Jul 11, 2019
Merged
@lizan lizan mentioned this pull request Jul 17, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
3 more reviewers

@jmillikin-stripe jmillikin-stripe jmillikin-stripe left review comments

@PiotrSikora PiotrSikora PiotrSikora approved these changes

@htuch htuch htuch approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@htuch htuch

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lizan @mattklein123 @htuch @PiotrSikora @jmillikin-stripe