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

Allow configuration through .clang-format files #1759

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

EmielMatthys
Copy link

@EmielMatthys EmielMatthys commented Jul 16, 2023

Please DO NOT FORCE PUSH. Don't worry about messy history, it's easier to do code review if we can tell what happened after the review, and force pushing breaks that.

Please make sure that your PR allows edits from maintainers. Sometimes its faster for us to just fix something than it is to describe how to fix it.

Allow edits from maintainers

After creating the PR, please add a commit that adds a bullet-point under the [Unreleased] section of CHANGES.md, plugin-gradle/CHANGES.md, and plugin-maven/CHANGES.md which includes:

  • a summary of the change
  • either
    • a link to the issue you are resolving (for small changes)
    • a link to the PR you just created (for big changes likely to have discussion)

If your change only affects a build plugin, and not the lib, then you only need to update the plugin-foo/CHANGES.md for that plugin.

If your change affects lib in an end-user-visible way (fixing a bug, updating a version) then you need to update CHANGES.md for both the lib and all build plugins. Users of a build plugin shouldn't have to refer to lib to see changes that affect them.

This makes it easier for the maintainers to quickly release your changes :)

@@ -111,7 +111,7 @@ String format(ProcessRunner runner, String input, File file) throws IOException,
args = tmpArgs;
}
final String[] processArgs = args.toArray(new String[args.size() + 1]);
processArgs[processArgs.length - 1] = "--assume-filename=" + file.getName();
processArgs[processArgs.length - 1] = "--assume-filename=" + file.getAbsolutePath();
Copy link
Member

Choose a reason for hiding this comment

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

Interesting, this is all it takes for clang to find the .clang-format file?

The only problem is that the content of the .clang-format files is not being captured (ideally by FileSignature). As currently implemented the up-to-date checking would break. What do you think about clangFormat().dotfile('.clang-fomat') as an API? Is there usually only one .clang-format file, or are there sometimes multiple?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the reply.
While newer versions of clang-format allow to specify file path for the dot file, I think older versions only support --style=file which tells clang-format to recursively look for such a dot file in parent directories (of targeted .c/.h/... file) and uses first one it can find.
I'll look into it and will try your API suggestion + FileSignature thingy 👍 .

Copy link
Member

Choose a reason for hiding this comment

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

If clang is actually searching with a recursive search, and we're specifying by pointing to a dotfile, I think that's okay. Of course there is a chance of some misconfiguration, but it's a lot better than giving the user no chance to capture the dotfile state at all.

Copy link
Author

@EmielMatthys EmielMatthys Jul 21, 2023

Choose a reason for hiding this comment

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

So looking into this, I learned 2 things so far: 🤓

  1. According to the ClangFormat docs, --style=file is actually the default, meaning CF will already pickup on present dotfiles in current version of PR if you do not provide a .style(...).

    • I tried this locally with CF 14,13,12,11 on Ubuntu. My PR uses the dotfile without adding .style('file') (still need to trigger it by modifying the .c file though)
  2. As of clang 14, the option also supports providing a path to a specific dot file -style=file:<format_file_path>

That also means that for the devs who are currently using spotless without .style and have a .clang-format file in the directory (or a parent directory) of the file being formatted, this PR will cause the dotfile to be picked up.
Maybe that's fine but it's an impact you should be aware of.

Two remaining questions for you:

  • Should I still look into detecting the dotfile changes? I can try if you want but not sure how long that will take me.
  • If we wanna support clang14's format_file_path, should Spotless perform a version check, let older clang versions complain/error, or catch and wrap the older clang error?
    • Current state of PR, providing .style('file:/path/to/dotfile') on older CF results in : stderr: Invalid value for -style

Copy link
Member

Choose a reason for hiding this comment

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

If this

static class State implements Serializable {
private static final long serialVersionUID = -1825662356883926318L;
// used for up-to-date checks and caching
final String version;
final @Nullable String style;
final transient ForeignExe exe;
// used for executing
private transient @Nullable List<String> args;
has a field FileSignature dotFile then it will detect dotfile changes. Right now, without this PR, I believe it will not detect dotfile changes becuase it doesn't know where the files are, so it can't find them even if they are there.

In terms of what to merge/implement, the most important thing is that it works for you. If you need to bump the required clang, we can bump it, and if someone needs support for older versions, they can add it.

Seems to me like a good compromise is if (dotFile == null) { currentBehaivorBeforePR } else { clang14 format_file_path }.

@nedtwigg
Copy link
Member

No rush, but I expect to make a release this week. Just FYI in case you are eager to have this in a published release.

@EmielMatthys
Copy link
Author

I'm currently very low on personal time 😅 so please don't wait for me. I quickly tried adding a FileSignature locally, updating it with the .clang-format hash, but changes were still not picked up.
I'll push my changes here later and ask for some help :)

Copy link
Author

@EmielMatthys EmielMatthys left a comment

Choose a reason for hiding this comment

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

Note to self: remove debug logs

Comment on lines +123 to +145
private void resolveDotFile(File targetFile) throws IOException
{
// The dot file is irrelevant if a specific style other than "file" is supplied.
if (style != null && !style.equals("file")) {
return;
}

File directory = targetFile.getParentFile();
Optional<File> dotFile = Optional.empty();
while (dotFile.isEmpty() && readableDirectory(directory)) {
dotFile = Arrays.stream(directory.listFiles()).filter(file -> file.getName().equals(DOT_FILE_NAME)).findAny();
directory = directory.getParentFile();
}

System.out.println("dotFile: " + dotFile);
// Every target file can have a different .clang-format file (in theory).
// Keep track of the ones we've covered and build the sig as we go.
if (dotFile.isPresent() && !dotFiles.contains(dotFile.get())) {
dotFiles.add(dotFile.get());
dotFileSig = FileSignature.signAsSet(dotFiles);
}
System.out.println("Signature" + dotFileSig);
}
Copy link
Author

Choose a reason for hiding this comment

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

This does not seem sufficient to detect .clang-format changes

@nedtwigg nedtwigg added pr-archive PRs which are still valid but have gotten stuck for some reason waiting on PR submitter labels Feb 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-archive PRs which are still valid but have gotten stuck for some reason waiting on PR submitter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants