8000 Fix unreachable clause where both tmp_plugin and tmp_plugin_instance are non-empty. by qingling128 · Pull Request #3350 · collectd/collectd · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix unreachable clause where both tmp_plugin and tmp_plugin_instance are non-empty. #3350

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 2 commits into from
Jan 7, 2020
Merged

Fix unreachable clause where both tmp_plugin and tmp_plugin_instance are non-empty. #3350

merged 2 commits into from
Jan 7, 2020

Conversation

qingling128
Copy link
Contributor
@qingling128 qingling128 commented Nov 27, 2019 •
edited
Loading

ChangeLog: Fix unreachable clause where both tmp_plugin and tmp_plugin_instance are non-empty.

qingling128
qingling128 commented Nov 27, 2019
View reviewed changes
src/aggregation.c
ssnprintf(inst->ident.plugin_instance,
sizeof(inst->ident.plugin_instance), "%s-%s",
tmp_plugin_instance, AGG_FUNC_PLACEHOLDER);
// Both tmp_plugin and tmp_plugin_instance are non-empty.
else
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This else clause is not reachable before this fix.

@qingling128 qingling128 mentioned this pull request Nov 27, 2019
Closed
kwiatrox
kwiatrox previously approved these changes Nov 29, 2019
View reviewed changes
Copy link
Member
@kwiatrox kwiatrox left a comment

Choose a reason for hiding this comment

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

Good fix from logical point of view.

@qingling128
Copy link
Contributor Author

@kwiatrox - Hi, any chance you know what the clang-format — unexpected status: 500 Internal Server Error failure is about? If it's intermittent, how can I trigger it again?

@rpv-tomsk
Copy link
Contributor

I become happy when I see red sign on clang-format CI.

@rpv-tomsk

This comment has been minimized.

Sign in to view
mrunge
mrunge previously approved these changes Dec 3, 2019
View reviewed changes
Copy link
Member
@mrunge mrunge left a comment

Choose a reason for hiding this comment

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

Thanks for the patch here!

@mrunge mrunge added the Automerge Labels PRs to be merged by a bot once approved label Dec 3, 2019
@mrunge mrunge added this to the 5.11.0 milestone Dec 3, 2019
@mrunge
Copy link
Member
mrunge commented Dec 3, 2019

@qingling128 Thank you for your patience and your patch. Unfortunately, I don't have an (easy) way to retrigger the check. The simplest way would be to push another change to your branch.

@qingling128 qingling128 dismissed stale reviews from mrunge and kwiatrox via c3de53a December 16, 2019 21:35
@qingling128
Copy link
Contributor Author

Just pushed some cosmetic changes to re-trigger the builds.

@mrunge
Copy link
Member
mrunge commented Dec 17, 2019

@qingling128 thank you!

Debian build fails with

usr/local/Cellar/grpc/1.25.0_1/include/grpcpp/impl/codegen/method_handler_impl.h:170:3: note: in instantiation of member function 'grpc_impl::internal::ServerStreamingHandler<collectd::Collectd::Service, collectd::QueryValuesRequest, collectd::QueryValuesResponse>::RunHandler' requested here

  ServerStreamingHandler(

  ^

collectd.grpc.pb.cc:78:11: note: in instantiation of member function 'grpc_impl::internal::ServerStreamingHandler<collectd::Collectd::Service, collectd::QueryValuesRequest, collectd::QueryValuesResponse>::ServerStreamingHandler' requested here

      new ::grpc::internal::ServerStreamingHandler< Collectd::Service, ::collectd::QueryValuesRequest, ::collectd::QueryValuesResponse>(

          ^

/usr/local/Cellar/protobuf/3.11.1/include/google/protobuf/message_lite.h:401:3: note: 'ByteSize' has been explicitly marked deprecated here

  PROTOBUF_DEPRECATED_MSG("Please use ByteSizeLong() instead")

  ^

/usr/local/Cellar/protobuf/3.11.1/include/google/protobuf/port_def.inc:153:53: note: expanded from macro 'PROTOBUF_DEPRECATED_MSG'

#define PROTOBUF_DEPRECATED_MSG(msg) __attribute__((deprecated(msg)))

                                                    ^

5 errors generated.

make[2]: *** [grpc_la-collectd.grpc.pb.lo] Error 1

make[1]: *** [all] Error 2

make: *** [distcheck] Error 1

The command "make distcheck DISTCHECK_CONFIGURE_FLAGS="--disable-dependency-tracking --enable-debug"" exited with 2.

This looks totally unrelated to the change :/

To solve that, we'll need some help from someone with permissions to fix CI.

@octo @mfournier could you have a look please, or provide

@rpv-tomsk

This comment has been minimized.

Sign in to view
This was referenced Dec 17, 2019
Merged
Closed
@qingling128
Copy link
Contributor Author

@mrunge - If the test failures are unrelated, can we disregard those failures?

@mrunge
Copy link
Member
mrunge commented Jan 4, 2020

@qingling128 I would, if I could. Unfortunately, CI failures prevent merging.

@qingling128
Copy link
Contributor Author

@mrunge Thanks for the clarification. Anything else I could do to move this forward? @octo @mfournier Would retrying the build help (assuming it's intermittent)?

@mrunge
Copy link
Member
mrunge commented Jan 5, 2020

The issue is not intermittent, I already kicked CI and it failed again.
Question is, why this patch fails, and others pass.
@octo and @mfournier are the people with powers on CI, I know @faxm0dem kindly offered his help, but has no permissions yet.

@qingling128
Copy link
Contributor Author
8000

The error seems to be:

CXX      grpc_la-collectd.grpc.pb.lo
In file included from collectd.grpc.pb.cc:6:
In file included from ./collectd.grpc.pb.h:42:
/usr/local/Cellar/grpc/1.25.0_1/include/grpcpp/impl/codegen/proto_utils.h:52:23: error: 'ByteSize' is deprecated: Please use ByteSizeLong() instead [-Werror,-Wdeprecated-declarations]
  int byte_size = msg.ByteSize();
                      ^
/usr/local/Cellar/protobuf/3.11.1/include/google/protobuf/message_lite.h:401:3: note: 'ByteSize' has been explicitly marked deprecated here
  PROTOBUF_DEPRECATED_MSG("Please use ByteSizeLong() instead")
  ^
/usr/local/Cellar/protobuf/3.11.1/include/google/protobuf/port_def.inc:153:53: note: expanded from macro 'PROTOBUF_DEPRECATED_MSG'
#define PROTOBUF_DEPRECATED_MSG(msg) __attribute__((deprecated(msg)))
                                                    ^

And the error is exclusive to the job with Xcode: xcode11.2.

I just saw this commit #3353 that disabled Xcode and it's already merged to master. Let me try rebasing.

@qingling128
Copy link
Contributor Author

@mrunge - Looks like rebasing made the checks passed. Would you mind LGTM this one and getting it merged?

@mrunge mrunge merged commit d56aca9 into collectd:master Jan 7, 2020
@mrunge
Copy link
Member
mrunge commented Jan 7, 2020
9FC2

Thank you and sorry for having this loong delay.

@qingling128
Copy link
Contributor Author

@mrunge - No worries. Thank you for the help!

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

@mrunge mrunge mrunge approved these changes

@kwiatrox kwiatrox kwiatrox left review comments

Assignees
No one assigned
Labels
Automerge Labels PRs to be merged by a bot once approved
Projects
None yet
Milestone
5.11
Development

Successfully merging this pull request may close these issues.
4 participants
@qingling128 @rpv-tomsk @mrunge @kwiatrox
0