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

Fix for old clang-format #199

Merged
merged 4 commits into from
Jul 22, 2024
Merged

Fix for old clang-format #199

merged 4 commits into from
Jul 22, 2024

Conversation

JohanBertrand
Copy link
Contributor

@JohanBertrand JohanBertrand commented Apr 28, 2024

Related Issue(s)
Has Unit Tests (y/n)
Documentation Included (y/n)

Change Description

Clang-format did not support a file path in its --style=file parameter before clang-format 14.0.

The current default clang format on ubuntu 20.04 is 10.0, and is not compatible with the current version of the tool, which is (I guess), responsible of some issues for users (see nasa/fprime#1101 (reply in thread))

This MR restores compatibility with older clang-format versions.

I also added distinction between verbose and quiet options for the formatter.

Rationale

Restore compatibility with older clang-format versions

@@ -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

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!

@JohanBertrand
Copy link
Contributor Author

I reverted a change in the subprocess.run() call, since my change did not have any impact on the execution of the function

@thomas-bc thomas-bc mentioned this pull request May 28, 2024
2 tasks
@JohanBertrand
Copy link
Contributor Author

@LeStarch @thomas-bc Is there anything you would want me to do here before merging this PR?

@LeStarch
Copy link
Collaborator

@thomas-bc I am ok with the change. Your thoughts?

@thomas-bc
Copy link
Contributor

I am ok with those changes. Sorry for the delay. Let's implement the following in another PR:

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.

@thomas-bc thomas-bc merged commit 9da5c8d into nasa:devel Jul 22, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants