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.

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

Tests are now being executed during gcc builds #10236

Merged
merged 42 commits into from
Jul 10, 2020
Merged

Tests are now being executed during gcc builds #10236

merged 42 commits into from
Jul 10, 2020

Conversation

dmitri-d
Copy link
Contributor

@dmitri-d dmitri-d commented Mar 3, 2020

This [re]enables tests during gcc builds.

Please note this PR shouldn't ve merged before envoyproxy/envoy-build-tools#34.

I'm not sure if it's ok to use bazel.gcc target, or a new target should be added, decided to use bazel.gcc in this PR.

@lizan
Copy link
Member

lizan commented Mar 3, 2020

@dmitri-d
Copy link
Contributor Author

dmitri-d commented Mar 9, 2020

@lizan: could you restart the tests please?

@lizan
Copy link
Member

lizan commented Mar 12, 2020

/azp run envoy-linux

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dmitri-d
Copy link
Contributor Author

dmitri-d commented Mar 17, 2020

There's two failures due to memory leaks:

*** WARNING: Cannot convert addresses to symbols in output below.
*** Reason: Cannot find 'pprof' (is PPROF_PATH set correctly?)
*** If you cannot fix this, try running pprof directly.
Leak of 8 bytes in 1 objects allocated from:
	@ 40ac9c 
	@ b4b8ad 

I tracked them down to static initialiser here: https://github.com/envoyproxy/envoy/blob/master/source/extensions/common/crypto/utility_impl.cc#L105. Not sure what to do about these other than to pass "HEAPCHECK=minimal" to tests. @lizan: WDYT?

Another failure due to quiche build failure (#10422).

@stale
Copy link

stale bot commented Mar 24, 2020

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 Mar 24, 2020
@dmitri-d
Copy link
Contributor Author

ping

@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Mar 24, 2020
@lizan
Copy link
Member

lizan commented Mar 25, 2020

@dmitri-d you need to do the build image update as well to pick up gcc-9, so this can't be in until #10506 is merged.

@dmitri-d
Copy link
Contributor Author

@lizan: could you restart the tests pls?

@lizan
Copy link
Member

lizan commented Mar 26, 2020

@dmitri-d you need merge master.

@dmitri-d
Copy link
Contributor Author

  • merged in master

@lizan
Copy link
Member

lizan commented Mar 30, 2020

some QUIC test failed to build in gcc. cc @danzh2010

@dmitri-d
Copy link
Contributor Author

@lizan: the build fails due to #10422.

@stale
Copy link

stale bot commented Apr 7, 2020

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 Apr 7, 2020
@stale
Copy link

stale bot commented Apr 14, 2020

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!

@dmitri-d
Copy link
Contributor Author

@lizan: could you re-open the PR please? At least one of the quiche-related issues has been resolved.

@lizan lizan reopened this May 13, 2020
@stale stale bot removed stale stalebot believes this issue/PR has not been touched recently labels May 13, 2020
@twghu
Copy link
Contributor

twghu commented Jun 26, 2020

Refer to envoyproxy/nighthawk#382 (comment) for explanation of problem.

@stale
Copy link

stale bot commented Jul 3, 2020

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 3, 2020
@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jul 6, 2020
Dmitri Dolguikh added 2 commits July 6, 2020 15:59
…uic_listeners/quiche/test_utils.h

Signed-off-by: Dmitri Dolguikh <[email protected]>
…:LocalRateLimitFilter::TestOneProtoInput

Signed-off-by: Dmitri Dolguikh <[email protected]>
@dmitri-d dmitri-d requested a review from junr03 as a code owner July 7, 2020 00:02
@@ -94,6 +94,10 @@ class MockGrpcMux : public GrpcMux {
MockGrpcMux();
~MockGrpcMux() override;

using GrpcMux::pause;
Copy link
Member

Choose a reason for hiding this comment

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

why did you need this? Does marking MOCK_METHOD below override in 4th parameter (like below) fixes gcc build?

MOCK_METHOD(void, start, (), (override));

@@ -54,4 +54,7 @@ namespace Envoy {
#define FALLTHRU
#endif

#if (defined(__GNUC__) && !defined(__clang__))
Copy link
Member

Choose a reason for hiding this comment

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

This should be in include/envoy/common/platform.h?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, the comment in platform.h says:

"platform.h" header exists to simplify the most common references
to non-ANSI C/C++ headers, required on Windows, Posix, Linux, BSD etc,
and to provide substitute definitions when absolutely required.

feels like not a good spot to me for the macro in question?

Copy link
Member

Choose a reason for hiding this comment

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

OK, I don't feel strongly either.

@dmitri-d
Copy link
Contributor Author

dmitri-d commented Jul 8, 2020

  • Added 'override' to MockGrpcMux methods implementing the interface

@dmitri-d
Copy link
Contributor Author

dmitri-d commented Jul 9, 2020

@lizan: Not sure why asan failed (all tests passed), otherwise looks good.

@dmitri-d
Copy link
Contributor Author

dmitri-d commented Jul 9, 2020

  • resolved conflict, merged in latest changes

@dmitri-d
Copy link
Contributor Author

@lizan, the build is green now.

@lizan lizan merged commit f2306b3 into envoyproxy:master Jul 10, 2020
@dmitri-d
Copy link
Contributor Author

Thank you very much for the reviews and feedback, @lizan.

@dmitri-d dmitri-d deleted the enable-gcc-tests branch July 10, 2020 19:29
scheler pushed a commit to scheler/envoy that referenced this pull request Aug 4, 2020
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