Make it easy to integrate native formatters

[nedtwigg]

This PR is a framework for finding native formatters, shelling out to them, and determining their version. By pinning their version, we can safely use all of our up-to-dateness and caching infrastructure. We gracefully handle the following situations:

• can't find the native executable
• we found the native executable but it's the wrong version
• we shelled out to the native executable, but it errored-out or otherwise provided unexpected output

For all of these cases, we provide actionable advice for the user, along with a complete snapshot of what went wrong with the native executable (we called with these args, we got this response, etc). It would be even better if we could install and version these formatters for the user, but that can be tackled in future PRs.

Here is how the code works:

• ProcessRunner makes it easy to efficiently and debuggably call foreign executables, and stuff their stdout into strings.
• ForeignExe finds executables on the path (or other strategies), and confirms that they have the correct version (to facilitate Spotless' caching). If the executable is not present or the wrong version, it points the user towards how to fix the problem.
• The above two classes were used to add support for python black...

  • spotless/lib/src/main/java/com/diffplug/spotless/python/BlackStep.java

    Lines 63 to 68 in 608e128

    	String exeAbsPath = ForeignExe.nameAndVersion("black", version)
    	.pathToExe(pathToExe)
    	.fixCantFind("Try running `pip install black=={version}`, or else tell Spotless where it is with `black().pathToExe('path/to/executable')`")
    	.fixWrongVersion("Try running `pip install --force-reinstall black=={version}`, or else specify `black('{versionFound}')` to Spotless")
    	.confirmVersionAndGetAbsolutePath();
    	return new State(this, exeAbsPath);

  • spotless/lib/src/main/java/com/diffplug/spotless/python/BlackStep.java

    Lines 84 to 91 in 608e128

    	String format(ProcessRunner runner, String input) throws IOException, InterruptedException {
    	return runner.exec(input.getBytes(StandardCharsets.UTF_8), args).assertExitZero(StandardCharsets.UTF_8);
    	}
    	FormatterFunc.Closeable toFunc() {
    	ProcessRunner runner = new ProcessRunner();
    	return FormatterFunc.Closeable.of(runner, this::format);
    	}

• ...and also for clang-format.

  • spotless/lib/src/main/java/com/diffplug/spotless/cpp/ClangFormatStep.java

    Lines 70 to 83 in 608e128

    	String howToInstall = "" +
    	"You can download clang-format from https://releases.llvm.org and " +
    	"then point Spotless to it with `pathToExe('/path/to/clang-format')` " +
    	"or you can use your platform's package manager:" +
    	"\n win: choco install llvm --version {version} (try dropping version if it fails)" +
    	"\n mac: brew install clang-format (TODO: how to specify version?)" +
    	"\n linux: apt install clang-format (try clang-format-{version} with dropped minor versions)";
    	String exeAbsPath = ForeignExe.nameAndVersion("clang-format", version)
    	.pathToExe(pathToExe)
    	.fixCantFind(howToInstall)
    	.fixWrongVersion(
    	"You can tell Spotless to use the version you already have with `clangFormat('{versionFound}')`" +
    	"or you can download the currently specified version, {version}.\n" + howToInstall)
    	.confirmVersionAndGetAbsolutePath();

  • spotless/lib/src/main/java/com/diffplug/spotless/cpp/ClangFormatStep.java

    Lines 106 to 116 in 608e128

    	String format(ProcessRunner runner, String input, File file) throws IOException, InterruptedException {
    	String[] processArgs = args.toArray(new String[args.size() + 1]);
    	// add an argument to the end
    	processArgs[args.size()] = "--assume-filename=" + file.getName();
    	return runner.exec(input.getBytes(StandardCharsets.UTF_8), processArgs).assertExitZero(StandardCharsets.UTF_8);
    	}
    	FormatterFunc.Closeable toFunc() {
    	ProcessRunner runner = new ProcessRunner();
    	return FormatterFunc.Closeable.of(runner, this::format);
    	}

• Incidental to this effort, FormatterFunc.Closeable now has new methods which make resource-handling safer. The old method is still available as ofDangerous, but it should not be used outside of a testing situation. There are some legacy usages of ofDangerous in the codebase, and it would be nice to fix them, but the existing usages are using it safely.

[nedtwigg]

Shipped in plugin-gradle 5.2.0.

[jbduncan]

Wow, I can see you put a lot of work into this PR, so very well done @nedtwigg! 👏

There's just one minor documentation mistake that I found and a query I have regarding some new tests, but other that, this LGTM.

[jbduncan]

As far as I can tell, these new Test tasks are not being run on CircleCI yet (although I'd love to be corrected on this!), and regardless I think we'd need Node.js, Python and Clang installed on our CircleCI setup for those tests to work, and I can't see any CircleCI-related changes in this PR, so I'm a little confused. Thoughts?

[nedtwigg]

Another good catch! None of this is being exercised on CircleCI right now, which is a problem. I did extensive testing on my personal win and mac machines, but this should definitely be tested better.

It's easy to write a cross-platform "machine preparation script" for black, but it's a little bit trickier for clang-format. On mac, brew gives you 10.0.1 and its hard to get anything else. On windows, choco gives you 10.0.0, and it's hard to get anything else. And those will both change over time - the llvm website seems to advertise for 12.0, which apparently hasn't made it into package managers yet. If your team is all on the same platform (fairly common), then none of this is a problem: "just run brew install clang-format"

It would be easy to make a docker image which has been prepared for specific versions for running blackTest and clangTest. However, if a contributor wants to run these tests, they'll have to do that preparation manually, which is a pain. When I went back and forth between win and mac, I just changed the versions in the tests, which isn't great.

Testing would be a lot easier if #673 and #674 were implemented. So my testing plan was: when someone has a good idea/PR for improving either of those issues, then that's when we'll run the tests on CI, since it'll be a lot easier at that point.

In the meantime, I'll commit to be being the human-test-runner for PRs which affect these native formatters, and I'm very open to a PR which adds a prepared docker image to offload that burden onto CI :D

[nedtwigg]

As always, thanks very much for the review @jbduncan, very helpful and much appreciated 😃

[jbduncan]

@nedtwigg Sorry for the late reply, you're very welcome. ☺️

I acknowledge that enabling CI for the tests for the native formatters is hard, given the lack of OS package managers' ability to download specific versions of dependencies. So I'm happy enough to leave them as a manual process! Thanks for enlightening me.