I would rather have fewer submodules than more. But apart from that, pulling this in this way would include it in the distributed source tarballs, which isn't good. Also, it's GPL. So we should really keep it at arm's length. Better just install it as a normal build dependency.

The main problem with installing a system uncrustify version through apt/yum/brew is that it is important that everyone uses the exact same version. A newer version might have a formatting bug fixed and result in slightly different formatting. So a submodule seemed like a reasonable way to ensure a consistent version. (for black this is less of an issue, because we can just say we should use the latest version, which is very easy to install using pip)

But apart from that, pulling this in this way would include it in the distributed source tarballs, which isn't good.

You mean the ones created by make dist? Since it's not in the EXTRA_DIST list they won't be included there. Also github doesn't seem to include submodule contents in the source tarballs it generates.

Also, it's GPL. So we should really keep it at arm's length. Better just install it as a normal build dependency.

Since we're not linking against it (only executing the built binary) I personally don't see much of an issue with it.

To resolve concerns though, I now pushed a version where we get the tarball from github and compile the uncrustify binary in a temporary directory.

We can just say, we use uncrustify version x.y.z, and everyone just has to get that one.

I hate git submodules and I don't want more git submodules. :)

Did you see I updated the the PR to remove the submodule and have the make target fetch and compile the uncrustify version from sources in a temporary directory? Is that okay? Or would you rather I remove the install steps from the Makefile completely and instead move them to a CONTRIBUTING.md and have people run them manually?

Seems like flake8 isn't really related to indenting.

I would like to keep the Python style check separate from the formatting checks.

Did you mean Python and C separate? and/or did you mean separating flake8 (which does not format automatically) from isort & black?

I split stuff up a bit more. Is this what you had in mind?

unrelated to this patch?

uncrustify directory had a python file that caused some issues with this I think

Yeah, I don't want uncrustify's stuff in pgbouncer's tree. We have enough crap in there, we don't need to import someone else's crap too and then keep track of that as versions change.

Yeah fair enough, I had already updated the PR to have a make target to compile it in a temporary directory. But I forgot to remove these changes. I did that now. And I created a separate PR to update the excludes for the already existing submodules: #924

Also, it would be good to make separate targets for C code and Python code. Most people will only deal with the C code, so it would be annoying to make them download black and isort, or manually copy the uncrustify command.