Skip to content

Conversation

@lizan
Copy link
Member

@lizan lizan commented Oct 23, 2019

Signed-off-by: Lizan Zhou [email protected]

Signed-off-by: Lizan Zhou <[email protected]>
@lizan lizan requested a review from keith October 23, 2019 17:29
keith
keith previously approved these changes Oct 23, 2019
Signed-off-by: Lizan Zhou <[email protected]>
htuch
htuch previously approved these changes Oct 24, 2019
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.

If this works and docs still build etc. then ship it. I have no idea why it is so inelegant to be passing file paths around in Bazel aspects, so basically "as long as it works" is the best we can do.

lizan added 2 commits October 24, 2019 16:32
Signed-off-by: Lizan Zhou <[email protected]>
Signed-off-by: Lizan Zhou <[email protected]>
"--enable-frame-pointers",
"--disable-libunwind",
],
] + select({
Copy link
Member Author

Choose a reason for hiding this comment

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

@keith I believe this is workaround for bazelbuild/bazel#9154

Copy link
Member

Choose a reason for hiding this comment

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

We should update rules_foriegn_cc instead where this is fixed

Copy link
Member Author

Choose a reason for hiding this comment

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

@keith No that doesn't fix, I guess rules_foreign_cc only fixed cmake, not configure_make, I still need this one.

Copy link
Member

Choose a reason for hiding this comment

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

We should fix it there too then 😞

lizan added 3 commits October 24, 2019 17:19
Signed-off-by: Lizan Zhou <[email protected]>
Signed-off-by: Lizan Zhou <[email protected]>
This reverts commit 5ff7bc8.

Signed-off-by: Lizan Zhou <[email protected]>
@lizan lizan marked this pull request as ready for review October 24, 2019 18:16
@@ -1,16 +1,36 @@
diff --git a/mocktracer/BUILD b/mocktracer/BUILD
Copy link
Member

Choose a reason for hiding this comment

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

Can we get this fixed upstream? CC @g-easy

Copy link
Contributor

Choose a reason for hiding this comment

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

CC @rnburn for OpenTracing ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

@lizan
Copy link
Member Author

lizan commented Oct 25, 2019

@htuch let's not block on the upstream patches?

@htuch
Copy link
Member

htuch commented Oct 25, 2019

@lizan I'm inclined as a general principle to make a best effort to merge patches upstream first. Not a huge fan of building up patches. But, if you think this is time sensitive, or is blocking something else, I'm cool with making that async.

@lizan
Copy link
Member Author

lizan commented Oct 25, 2019

@htuch agreed on upstream first, but I'd also like to get bazel up-to-date before stable release.

Copy link
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.

Agreed if we can't land the patches by Monday let's land this before we cut 1.12.0

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.

Sure, let's get this in for 1.12.0.

@lizan lizan merged commit e004be6 into envoyproxy:master Oct 28, 2019
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.

5 participants