-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Shorten adaptive_concurrency/concurrency_controller paths #10560
Shorten adaptive_concurrency/concurrency_controller paths #10560
Conversation
- having paths greater than 240 characters breaks compilation on Windows with errors indicating header files do not exist in the includes directory - the adaptive_concurrency/concurrency_controller name is repetitive, this change shortens it to adaptive_concurrency/controller to get past Windows file path limits - namespace changed to AdaptiveConcurrency::Controller to mimic same shortening, AdaptiveConcurrency::Controller::ConcurrencyController class is kept as that seems an appropriately descriptive name for the class Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io> Signed-off-by: William A Rowe Jr <wrowe@pivotal.io> Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io> Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>
Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io> Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>
Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io> Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>
Requesting additional review by @lizan per discussion of PR 10542 |
bazel clang-tidy and go_control_plane_mirror both look unrelated to this patch... envoy-macos doesn't have a great track record. Leaving this PR as completed and considering the CI passing from our perspective. |
/retest error in circle ci: |
🔨 rebuilding |
Patch looks fine to me, but why this is causing problems related to the 260 character path size limit in Windows? Those include paths are less than 100 characters, so I don't understand how shaving off another ~20 chars fixes this? |
@tonya11en its not the include path in the file, it’s the resulting bazel build tree that’s the issue and the file path that ends up in the cl.exe params file, e.g the include param for the gradient_controller.h file becomes: Shaving down the redundant |
Please note that this PR is blocking #10542 so accelerated review and merge (to simplify that PR) would be most appreciated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no reason not to merge this, so I'll approve. However, I'm confused as to how other instances of long path names will be prevented from breaking the Windows build. Is this going to be enforced somehow?
I'd really like to see some documentation in the tree explaining the windows path name requirements. Otherwise, Windows build CI failures will be pretty vexing for people who are unaware of the limitation. I'd like to see it written before merging this change, but since you're blocked I'll defer to @mattklein123 as to whether this can be merged without an explanation in the docs.
I'm OK merging this but I agree that we will need to do better here in terms of blocking changes that cause this issue and/or making it more obvious in the future. |
PRs/commits will fail Windows compilation once #10542 is merged and will likely fail with error messages from the compiler mentioning |
@sunjayBhatia I think that will be very confusing and that people will go down a few different rabbit holes before looking in the Windows developer docs. There should be some kind of verification in the Windows build steps to ensure that these path lengths aren't exceeding the Windows filesystem limitations and to provide a failure message indicating the exact problem. |
Description:
Windows include paths must net out to no more than 240 characters in our observations.
This specific extension directory path is both redundant and breaks test compilation
on the Windows architecture. The C++ namespace is similarly redundant.
The controller is always identified in the context of the adaptive_concurrency scope.
The class name might be potentially ambiguous, so this is left as ConcurrencyController, while the namespace is simplified to AdaptiveConcurrency.Controller, but that aspect is up for discussion.
Risk Level: low
Testing: local
Docs Changes: n/a
Release Notes: n/a