Skip to content

fix: fill in the required args for Api::Impl #14554

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

Merged
mattklein123 merged 2 commits into from
Jan 10, 2021
Merged

fix: fill in the required args for Api::Impl #14554

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5f83c45
include random generator
ynqa Jan 5, 2021
eead850
clang-format: change the order for includes
ynqa Jan 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions tools/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,9 @@ envoy_cc_binary(
"//source/common/common:assert_lib",
"//source/common/protobuf:message_validator_lib",
"//source/common/protobuf:utility_lib",
"//source/common/common:random_generator_lib",
"//source/common/stats:isolated_store_lib",
"//test/test_common:test_version_linkstamp",
"@envoy_api//envoy/config/bootstrap/v2:pkg_cc_proto",
] + envoy_cc_platform_dep("//source/exe:platform_impl_lib"),
)
4 changes: 3 additions & 1 deletion tools/bootstrap2pb.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

#include "common/api/api_impl.h"
#include "common/common/assert.h"
#include "common/common/random_generator.h"
#include "common/event/real_time_system.h"
#include "common/protobuf/message_validator_impl.h"
#include "common/protobuf/utility.h"
Expand All @@ -31,8 +32,9 @@ int main(int argc, char** argv) {
Envoy::PlatformImpl platform_impl_;
Envoy::Stats::IsolatedStoreImpl stats_store;
Envoy::Event::RealTimeSystem time_system; // NO_CHECK_FORMAT(real_time)
Envoy::Random::RandomGeneratorImpl rand;
Envoy::Api::Impl api(platform_impl_.threadFactory(), stats_store, time_system,
platform_impl_.fileSystem());
platform_impl_.fileSystem(), rand);
Copy link
Contributor

@antoniovicente antoniovicente Jan 6, 2021

Choose a reason for hiding this comment

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

cc @akonradi @lizan @htuch

I wonder if it would be possible to have a basic test the exercises this tool and catches compile breakages like this one.

Copy link
Contributor

@akonradi akonradi Jan 6, 2021

Choose a reason for hiding this comment

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

The compilation breakage should have been caught by the existing build rule. Do we not try to build everything under //tools somewhere in CI?

Copy link
Member

@mattklein123 mattklein123 Jan 6, 2021

Choose a reason for hiding this comment

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

We must not which seems broken. Can you add this somewhere here? https://github.com/envoyproxy/envoy/blob/master/ci/do_ci.sh

/wait

Copy link
Contributor

@antoniovicente antoniovicente Jan 8, 2021

Choose a reason for hiding this comment

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

My understanding is that bazel test does not build binaries that are not required to run tests. In order to verify compilation of a binary, you need a test that depends on it.

Copy link
Member

@htuch htuch Jan 8, 2021

Choose a reason for hiding this comment

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

bazel build //tools/... should suffice.

Copy link
Member

@mattklein123 mattklein123 Jan 8, 2021

Choose a reason for hiding this comment

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

Yeah ^

Copy link
Contributor

@antoniovicente antoniovicente Jan 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think that the way to do this would be to change the following line in ci/build_setup.sh to also cover //tools/...
I can offer to send that as a separate change if we prefer just getting this build fix in and address the CI coverage issue separately.

[ -z "${ENVOY_BUILD_TARGET}" ] && export ENVOY_BUILD_TARGET=//source/exe:envoy-static //tools/...

Copy link
Contributor

@antoniovicente antoniovicente Jan 8, 2021

Choose a reason for hiding this comment

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

There are other build breakages under //tools/... like:

ERROR: tools/testdata/protoxform/envoy/v2/BUILD:23:14: //tools/testdata/protoxform/envoy/v2:freeze_protos: missing input file '//tools/testdata/protoxform/envoy/v2:active_non_terminal.proto'
ERROR: tools/testdata/protoxform/envoy/v2/BUILD:23:14 3 input file(s) do not exist

I'm looking into them.


envoy::config::bootstrap::v2::Bootstrap bootstrap;
Envoy::MessageUtil::loadFromFile(argv[1], bootstrap,
Expand Down