[beman-tidy] Implement DIRECTORY.INTERFACE_HEADERS check #134
Conversation
Add beman-tidy: The Codebase Bemanification Tool
- We no longer need Makefiles - No need for requirements.txt files, better lockfiles instead Signed-off-by: rishyak <[email protected]>
Signed-off-by: rishyak <[email protected]>
Signed-off-by: rishyak <[email protected]>
Signed-off-by: rishyak <[email protected]>
Signed-off-by: rishyak <[email protected]>
…alAction - `type` param of `argparse.BooleanOptionalAction` are deprecated and will be removed in Python 3.14 - https://docs.python.org/3.14/deprecations/pending-removal-in-3.14.html Signed-off-by: rishyak <[email protected]>
Authored-by: Darius Neațu <[email protected]>
- Move code under beman_tidy/ (including lib/) - This aligns with PyPA's recommended flat layout, and ensures your lib/ subpackage is bundled in the wheel. - See: https://packaging.python.org/en/latest/discussions/src-layout-vs-flat-layout/
beman-tidy: migrate to pyproject.toml-based build with Astral's uv
Exclude more build artifacts
[beman-tidy] tweaks in docs
…ly stored inside the beman_tidy/ subdirectory
…TRUNK_VERSION [beman-tidy] Implement RELEASE.*
…ment-REPOSITORY.DISALLOW_GIT_SUBMODULES [beman-tidy] Implemented REPOSITORY.DISALLOW_GIT_SUBMODULES
…all, run_beman_tidy_on_exemplar
| # Example path: "exemplar/include/beman/exemplar" | ||
| include_path = self.path / "include" / "beman" / self.repo_info["name"] | ||
| if ( | ||
| not os.path.exists(include_path) |
There was a problem hiding this comment.
still not applied - we should not require using os if we are using Path
|
|
||
| # TODO DIRECTORY.INTERFACE_HEADERS | ||
| @register_beman_standard_check("DIRECTORY.INTERFACE_HEADERS") | ||
| class DirectoryInterfaceHeadersCheck(DirectoryBaseCheck): |
There was a problem hiding this comment.
You should not have include_path = self.path / "include" / "beman" / self.repo_info["name"] in your code!
This is why BemanTreeDirectoryCheck class exists. We should have class DirectoryInterfaceHeadersCheck(BemanTreeDirectoryCheck), which will give you self.path -> include/beman/short_name. And then you can create other paths, if you want starting from self.repo_path.
| include_path = self.path / "include" / "beman" / self.repo_info["name"] | ||
| if ( | ||
| not os.path.exists(include_path) | ||
| or os.path.isfile(include_path) |
There was a problem hiding this comment.
| or os.path.isfile(include_path) | |
| or os.path.isfile(include_path) |
can we use pathlib instead os everywhere?
| or len(os.listdir(include_path)) == 0 | ||
| ): | ||
| self.log( | ||
| f"The path '{self.path}' does not exist, is a file or is empty." |
There was a problem hiding this comment.
| f"The path '{self.path}' does not exist, is a file or is empty." | |
| f"The path '{self.path}' does not exist, is a file or is empty. " |
| ): | ||
| self.log( | ||
| f"The path '{self.path}' does not exist, is a file or is empty." | ||
| " All public header files must reside within include/beman/<short_name>/." |
There was a problem hiding this comment.
| " All public header files must reside within include/beman/<short_name>/." | |
| "All public header files must reside within include/beman/<short_name>/." |
| if self.repo_info["name"] == "exemplar": | ||
| exclude_dirs.add("cookiecutter") | ||
|
|
||
| hpp_files = [] |
There was a problem hiding this comment.
| hpp_files = [] | |
| header_files = [] |
Please don't resolve comments , just push fixes and reply. I will marke comments as resolved e.g. https://github.com/bemanproject/infra/pull/134/files#r2207970309.
| for hpp_file in hpp_files: | ||
| if not hpp_file.startswith(str(include_path)): | ||
| self.log( | ||
| f"Header file '{hpp_file}' is not under include/beman/{self.repo_info['name']}/ directory." |
There was a problem hiding this comment.
| f"Header file '{hpp_file}' is not under include/beman/{self.repo_info['name']}/ directory." | |
| f"Header file '{hpp_file}' is not under include/beman/{self.repo_name}/ directory." |
we have that shortcut - pleas read tools/beman-tidy/beman_tidy/lib/checks/base/base_check.py
…install-and-run-beman-tidy [beman-tidy] CI flows should test, build, install and run beman-tidy
…ch-8 [beman-tidy] CI tests should run on schedule and on demand.
…ch-9 Tweaks in docs
…ment-DIRECTORY.DOCS [beman-tidy] implement DIRECTORY.DOCS
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
tools/beman-tidy/tests/lib/checks/beman_standard/directory/test_directory.py
Outdated
Show resolved
Hide resolved
|
|
||
| def check(self): | ||
| """ | ||
| Check that all public header files reside within the include/beman/<short_name>/ directory. |
There was a problem hiding this comment.
Again, let's not discussed implementation from this PR, as the pattern is similar to the one for PAPERS (https://github.com/bemanproject/infra/pull/159#discussion_r2223086644).
I propose a plan for DIRECTORY.INTERFACE_HEADERS and DIRECTORY.IMPLEMENTATION_HEADERS
@Valid header locations
First of all, we can have headers in
- include/beman/<short_name> - valid location
- include/ but not in include/beman/<short_name> - invalid location
- src/beman/short_name - valid location
- src but not in src/beman/short_name - invalid location
- examples/ - valid location
- tests/beman/short_name - valid location
- tests/ but not in tests/beman/short_name - invalid location
- cookiecutter - valid location
- infra - valid location
TLDR:
valid_header_locations = [
# Library specific
"include/beman/short_name",
"src/beman/short_name",
"tests/beman/short_name",
'examples",
# Infra
"infra",
"cookiecutter",
]We can assume, IMO, for now, that this is a complete set of locations which can be used in v1 implementation. If we go with this approach, we can actually run locally run the tool on all libraries and see if we are missing something before first commit!
@CML can provide us necessary info?
- I briefly investigated if we can use CML. As per my understanding, CML does not export this info in compile_commands.json, so I think we should actually parse the CML and find the
target_sourcescall and see what we set forPUBLICandPRIVATE. @nickelpro ? - I don't want to complicate things - let's assume it works with CML. What if a library requires custom build flags to actually build with a (sub)directory? etc
I think it's better to do best-effort search by accessing the filesystem, IMO.
=================================================================
My proposal
I. Steps for DIRECTORY.INTERFACE_HEADERS:
- We assert to exists
include/beman/<short_name>(we cannot have a library without at least one public header!). If invalid, we stop ->Missing include/beman/<short_name>. Expected public interface to be present. Please check .... - We do search for all headers in the repo which are not in
valid_header_locations, and if we find one we fail the check. - If 1 + 2 passed,
DIRECTORY.INTERFACE_HEADERScheck also passes.
(We actually just checked headers are at right location. We didn't use the "public" thing here. It's not needed IMO.)
TLDR: It's same proposal from this PR.
II. Steps for DIRECTORY.IMPLEMENTATION_HEADERS.
We cannot actually easier distinguish which is public and which is private headers. We can say it's SKIPPED and log a message (we assume my PR #148 is already merged).
=================================================================
Again, please ignore the current pushed implementation, let's decide what should be the desired approach.
CC: @vickgoodman (PR owner) @JeffGarland (Style Czar) @nickelpro (Build Czar)
There was a problem hiding this comment.
Due to Victor's availability (PTO), we discussed and I may take this PR (we want DIRECTORY.*) to be completed for release 1.0 ASAP. @vickgoodman , can you confirm?
There was a problem hiding this comment.
Feel free to update the PR and close the issue. Thanks!
There was a problem hiding this comment.
I will update this PR next weeks.
|
Reminder to update this PR with latest main after https://github.com/bemanproject/infra/pull/170. |
7 participants
Implement DIRECTORY.INTERFACE_HEADERS check - #43