-
Notifications
You must be signed in to change notification settings - Fork 553
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
Lint includes #17193
Lint includes #17193
Conversation
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.
will there be a way of executing this via task in vtools
, like task lint:includes
or something?
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.
+1 to moving this to tools/lint
or something. That's where I had started adding some ast grep for these sorts of checks.
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.
fixed. move it to tools as a separate linter.
new failures in https://buildkite.com/redpanda/redpanda/builds/46471#018e5933-6e7a-4faf-a092-dae8b5b1dd09:
new failures in https://buildkite.com/redpanda/redpanda/builds/46471#018e5943-be7e-49a0-be51-a89a727aa9c0:
|
if len(include_path.parents) <= 1: | ||
continue | ||
|
||
include_module = include_path.parents[-2] |
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.
This doesn't account for nested modules, you need something like [:-2]
?
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.
yeh, i didn't realize we had nested modules.
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.
handling this case now
Found module wasm/parser: src/v/wasm/parser
Validating module wasm/parser
Module root src/v/wasm/parser
Include root src/v/wasm/parser/include
Include path src/v/wasm/parser/include/wasm/parser
Found 1 exports: sample ['src/v/wasm/parser/include/wasm/parser/parser.h', '...']
Found 7 internal sample ['src/v/wasm/parser/CMakeLists.txt', 'src/v/wasm/parser/leb128.h', 'src/v/wasm/parser/tests/CMakeLists.txt', '...']
Found module random: src/v/random
...
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46471#018e5933-6e76-4ff6-9a16-afc2bac73d35 ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46517#018e5dcf-d9f8-4a05-a839-a7005e9699b6 ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46517#018e5dcf-d9fb-4227-ad21-e9e29db5d072 |
Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
Tooling like the includes linter assumes that test directories are named as plural 'tests' which fits with naming convention of a large majority of existing modules in the tree. Renaming the paths to avoid more special casing in tooling. Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
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.
🔥
@@ -0,0 +1,167 @@ | |||
#!/usr/bin/env python3 | |||
# Copyright 2020 Redpanda Data, Inc. |
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.
😄 Insert bad joke about getting used to writing the new year
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.
Yeh. Git knows the real date
Checks that new-style modules meet requirements, and that the rest of the tree doesn't violate public/private divide. - has <module>/include/<module>/* public headers - tests in a module can include anything in the module - includes outside a new-style module must only be public includes Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
Force pushed to fix python formatting! Sorry about the hassle there |
Checks that new-style modules meet requirements, and that the rest of
the tree doesn't violate public/private divide.
<module>/include/<module>/*
public headersBackports Required
Release Notes