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.

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

Fix for old clang-format #199

Merged
merged 4 commits into from
Jul 22, 2024
Merged

Fix for old clang-format #199

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dd42714
Fix for old clang format
JohanBertrand Apr 28, 2024
3fec940
Fix context for subprocess
JohanBertrand Apr 28, 2024
df7c372
Update verbose info
JohanBertrand Apr 28, 2024
ba8fa4d
Revert subprocess change
JohanBertrand May 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions src/fprime/util/code_formatter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ def __init__(self, executable: str, style_file: "Path", options: Dict):
self.style_file = style_file
self.backup = options.get("backup", True)
self.verbose = options.get("verbose", False)
self.quiet = options.get("quiet", False)
self.validate_extensions = options.get("validate_extensions", True)
self.allowed_extensions = ALLOWED_EXTENSIONS.copy()
self._files_to_format: List[Path] = []
Expand Down Expand Up @@ -143,11 +144,18 @@ def execute(
clang_args = [
self.executable,
"-i",
f"--style=file:{self.style_file}",
*(["--verbose"] if self.verbose else []),
f"--style=file",
Copy link
Contributor

@thomas-bc thomas-bc Apr 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

From clang-format docs:

When using -style=file, clang-format for each input file will try to find the .clang-format file located in the closest parent directory of the input file. When the standard input is used, the search is started from the current directory.

Since the fprime .clang-format lives in fprime/.clang-format, this will not be able to catch the format file if you are formatting a file outside of fprime/ (i.e. in your project, say in HelloWorldProject/Components/MyComponent/)

Ways I can think of to deal with this:

  • have .clang-format be copied to the project's root when running fprime-bootstrap project. This would mean projects could tweak with how they like their formatting, which may be good. But I am not sure I like the idea of duplicating the file...
  • if older version, warn and fallback to a default style
  • well.. I'm out of ideas

Let me know what you think! Or if you can think of better alternatives

Copy link
Collaborator

Choose a reason for hiding this comment

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

Am I missing something? I pulled up clang-format 10.0.0 documentation (https://releases.llvm.org/10.0.0/tools/clang/docs/ClangFormat.html) and it says that -style=file is supported.

  --style=<string>           - Coding style, currently supports:
                                 LLVM, Google, Chromium, Mozilla, WebKit.
                               Use -style=file to load style configuration from
                               .clang-format file located in one of the parent
                               directories of the source file (or current
                               directory for stdin).
                               Use -style="{key: value, ...}" to set specific
                               parameters, e.g.:
                                 -style="{BasedOnStyle: llvm, IndentWidth: 8}"

Copy link
Contributor

Choose a reason for hiding this comment

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

@LeStarch the difference is between --style=file, which recurses up directories until it finds a .clang-format and --style=file:<path-to-clang-format> which uses the file at the given path.

We currently use the latter, pointing to <project>/fprime/.clang-format

Copy link
Collaborator

Choose a reason for hiding this comment

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

@thomas-bc I see. The above means literally --style=file and the new is literally --style=file:/some/path now I see.

I am in-favor of cloning configuration out of fprime into projects (e.g. fprime/config -> project/config, fprime/.clang-format -> project/.clang-format, ect). Although this makes the upgrade path harder, it makes the customization story much easier.

If you @thomas-bc concur, we can merge this, and fix the project creation in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am also liking the first option, since it will allow third parties to use their own .clang-format and fprime-util format if needed.

I would have liked to have an option on fprime-util to specify the .clang-format file to be used as an optional parameter. This could work easily with version of clang-tidy >= 14.0, but not for the older versions. The only way I found to do so, is to copy temporarily the .clang-format file in the folder of the formatted file, but I don't think it's worth it.

If you think it is something worth having, I can later work on adding a version check and allow the user to specify the style file only if the version of clang-tidy is >= 14.0.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the behavior in this case? If it fails, then the solution is obvious: add a .clang-format. If it formats with the default clang-format, good it has a defined format.

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant to say --fallback-style from clang-format: https://clang.llvm.org/docs/ClangFormat.html

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we not just accept "LLVM" as a reasonable fallback style for projects that don't define a .clang-format? It is the default after all.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also do like your idea of having fprime-util format check the existence of a .clang-format file at the root of the project and if it doesn't exist -> automatically create one.
This gently pushes for best practice, and will ensure older projects get up to speed on this new file that should be at the root of the project.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wasn't suggesting that, but it is a good idea!

*(["--verbose"] if not self.quiet else []),
*pass_through,
*self._files_to_format,
]
if self.verbose:
print("[INFO] Clang format executable:")
print(f"[INFO] {self.executable}")
print("[INFO] Clang format arguments:")
print(f"[INFO] {clang_args[1:]}")
print("[INFO] Clang format style file:")
print(f"[INFO] {self.style_file}")
status = subprocess.run(clang_args)
self._postprocess_files()
return status.returncode
3 changes: 2 additions & 1 deletion src/fprime/util/commands.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,8 @@ def run_code_format(
____: unused pass-through arguments
"""
options = {
"verbose": not parsed.quiet,
"quiet": parsed.quiet,
"verbose": parsed.verbose,
"backup": not parsed.no_backup,
"validate_extensions": not parsed.force,
}
Expand Down
Loading