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

chore: Fully specify include paths for sessiond #12496

Merged
merged 1 commit into from Apr 21, 2022

Conversation

themarwhal
Copy link
Member

@themarwhal themarwhal commented Apr 19, 2022

Summary

This change was partially automated (by using Scott's full path refactoring script mentioned in #9961).

The changes are:

  1. (manually done) Use < over " for protobuf includes
  2. (partially automated with some manual fixes) Fix include directives to use the full path from magma root
  3. (automated) Apply IWYU by running dev_tools/apply-iwyu.sh lte/gateway/c/session_manager in the devcontainer

Officially closes #8494 (the stale PR bot closed this already but we can officially resolve it)

Test Plan

CI (which includes build and unit test checks)

Next change

We will then rename all header files from .h to .hpp to better differentiate between C and C++ header files. This was already done by @electronjoe for the rest of C/C++ codebase.

Additional Information

  • This change is backwards-breaking

@pull-request-size pull-request-size bot added the size/XL Denotes a Pull Request that changes 500-999 lines. label Apr 19, 2022
@github-actions
Copy link
Contributor

Thanks for opening a PR! 💯

A couple initial guidelines

Howto

  • Reviews. The "Reviewers" listed for this PR are the Magma maintainers who will shepherd it.
  • Checks. All required CI checks must pass before merge.
  • Merge. Once approved and passing CI checks, use the ready2merge label to indicate the maintainers can merge your PR.

More info

Please take a moment to read through the Magma project's

If this is your first Magma PR, also consider reading

@github-actions github-actions bot added the component: agw Access gateway-related issue label Apr 19, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Apr 19, 2022

feg-workflow

    2 files  202 suites   38s ⏱️
370 tests 370 ✔️ 0 💤 0
384 runs  384 ✔️ 0 💤 0

Results for commit 476b05f.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 19, 2022

agw-workflow

    2 files      2 suites   3m 37s ⏱️
592 tests 583 ✔️ 9 💤 0
593 runs  584 ✔️ 9 💤 0

Results for commit 476b05f.

♻️ This comment has been updated with latest results.

@pull-request-size pull-request-size bot added size/XXL Denotes a Pull Request that changes 1000+ lines. and removed size/XL Denotes a Pull Request that changes 500-999 lines. labels Apr 19, 2022
@themarwhal themarwhal force-pushed the fix-sessiond-paths branch 5 times, most recently from 418803c to 36558be Compare April 19, 2022 21:22
@themarwhal themarwhal marked this pull request as ready for review April 20, 2022 12:28
@themarwhal themarwhal requested review from a team, electronjoe and uri200 April 20, 2022 12:28
Copy link
Member

@electronjoe electronjoe left a comment

Choose a reason for hiding this comment

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

As it seems that IWYU is working for session_manager now (it is?) - could we turn up CI automation for IWYU for this sub-directory?

#include <glog/logging.h>
#include <grpcpp/channel.h>
#include <grpcpp/impl/codegen/status.h>
#include <feg/gateway/services/aaa/protos/accounting.grpc.pb.h>
Copy link
Member

Choose a reason for hiding this comment

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

Not looking to have you change your PR, but I'm wondering why these went to <> from ""? In the include fixup script I'm working on, I intended for repo-generated protos to use quotes (#12441).

Copy link
Member Author

Choose a reason for hiding this comment

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

ooo didn't realize. IWYU seems to default to <>, so I standardized on that. I don't have a preference either way as long as there is tooling around it to automate it :D

#include <cpp_redis/core/client.hpp>
#include <cpp_redis/core/reply.hpp>
#include <cpp_redis/misc/error.hpp>
#include <folly/Range.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we fix these include ordering linter errors.

Copy link
Member Author

Choose a reason for hiding this comment

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

The include ordering in this PR was done by the IWYU script and since there is no automated way of appeasing cpplint right now, I would like to ignore these cpplint warnings for now. The hope is to eventually have an include sorting script that handles this (see #12441).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok Marie! Then its fine.

@GANESH-WAVELABS
Copy link
Contributor

I have below minor comments.

  1. There were some cpp linter errors, Let us fix at least include order related errors wherever applicable.

  2. My concern is where does the *.pb.h or *.grpc.pb.h files to be included. Do we need to follow ([c/c++] Formatting tool for c/c++ Includes repo-wide #12441 ) and keep this at the end as mentioned in below image. Also these should be inside " " instead of < > as mentioned in one of comment in this PR.
    image

  3. Also shall we keep blank line for each distinct set of header files as mentioned in ([c/c++] Formatting tool for c/c++ Includes repo-wide #12441 )

Except above ordering / formatting issues, Changes Looks Good To Me!

@themarwhal
Copy link
Member Author

I have below minor comments.

  1. There were some cpp linter errors, Let us fix at least include order related errors wherever applicable.
  2. My concern is where does the *.pb.h or *.grpc.pb.h files to be included. Do we need to follow ([c/c++] Formatting tool for c/c++ Includes repo-wide #12441 ) and keep this at the end as mentioned in below image. Also these should be inside " " instead of < > as mentioned in one of comment in this PR.
    image
  3. Also shall we keep blank line for each distinct set of header files as mentioned in ([c/c++] Formatting tool for c/c++ Includes repo-wide #12441 )

Except above ordering / formatting issues, Changes Looks Good To Me!

Thanks @GANESH-WAVELABS ! Those points all sound reasonable, but I think it will be much easier to address them once we have a script that can automate those fixes. I believe @electronjoe is working on this right now :) As for protobuf includes, I was hoping that the formatting script would eventually handle modifying '<' to '"' as well. But if that is not the case, I can modify those in this PR.

@GANESH-WAVELABS
Copy link
Contributor

I have below minor comments.

  1. There were some cpp linter errors, Let us fix at least include order related errors wherever applicable.
  2. My concern is where does the *.pb.h or *.grpc.pb.h files to be included. Do we need to follow ([c/c++] Formatting tool for c/c++ Includes repo-wide #12441 ) and keep this at the end as mentioned in below image. Also these should be inside " " instead of < > as mentioned in one of comment in this PR.
    image
  3. Also shall we keep blank line for each distinct set of header files as mentioned in ([c/c++] Formatting tool for c/c++ Includes repo-wide #12441 )

Except above ordering / formatting issues, Changes Looks Good To Me!

Thanks @GANESH-WAVELABS ! Those points all sound reasonable, but I think it will be much easier to address them once we have a script that can automate those fixes. I believe @electronjoe is working on this right now :) As for protobuf includes, I was hoping that the formatting script would eventually handle modifying '<' to '"' as well. But if that is not the case, I can modify those in this PR.

Thanks Marie!! PR LGTM.

Signed-off-by: GitHub <noreply@github.com>
@themarwhal themarwhal force-pushed the fix-sessiond-paths branch 2 times, most recently from d44b39e to 476b05f Compare April 21, 2022 14:28
@themarwhal themarwhal enabled auto-merge (squash) April 21, 2022 15:12
@themarwhal themarwhal merged commit e3f3ca9 into magma:master Apr 21, 2022
@themarwhal themarwhal deleted the fix-sessiond-paths branch April 21, 2022 15:30
emakeev pushed a commit to emakeev/magma that referenced this pull request Aug 5, 2022
Signed-off-by: GitHub <noreply@github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: agw Access gateway-related issue size/XXL Denotes a Pull Request that changes 1000+ lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

techdebt: SessionD unit tests should use full path to access SessionD files
3 participants