supercollider
supercollider copied to clipboard
supercollider
Bump to `clang-format-14`
Purpose and Motivation
llvm-8 is not supported by macOS 13+ anymore, therefore it is nearly impossible to apply the necessary formatting rules under an up-to-date macOS.
This PR bumps clang-format to version 14 which should be available for most OS and most architectures.
This is a more "conservative" PR which only bumps clang-format to version 14 and does not change how the linting/formatting is applied. See #6096 which was the initial point of this PR, but used pre-commit to apply formatting and obtain a clang-format binary.
Types of changes
- Bug fix
- Breaking change
Changes performed
- Bumped ubuntu version to 22.04 for linting CI b/c clang-format-14 is not available via apt on 20.04
- Added new formatting rules in order to keep linting noise as low as possible
To-do list
- [ ] Code is tested
- [ ] All tests are passing
- [ ] Updated documentation
- [x] This PR is ready for review
Replying to https://github.com/supercollider/supercollider/pull/6096#issuecomment-1892564709
Thanks for your feedback @mossheim!
- You can download packages for clang from the LLVM website so there is no need to be super limited about what is in various package managers.
It seems llvm stopped releasing binaries for intel macs (I'm a bit surprised about this, but I am not the only one). 14.0.6 seems to be the last official version from the official suppliers, but it is even not notarized.
Also, having the formatter not available through a package manager makes the CI more delicate, see here.
- Can you test whether there any difference between 14 and 16 on our codebase using your new .clang-format?
B/c of the above I don't know how to setup clang-format 14+ on my machine.
It looks like brew has 17 for Intel, can you test with that? https://formulae.brew.sh/formula/clang-format#default
You can get newer versions on Ubuntu very easily thru llvm's PPA: https://apt.llvm.org/
It looks like brew has 17 for Intel, can you test with that? https://formulae.brew.sh/formula/clang-format#default
You can get newer versions on Ubuntu very easily thru llvm's PPA: https://apt.llvm.org/
The main problem I see is that clang does not issue LTS versions, which would make life so much easier? So the closest thing to an LTS would be to use the default version of the latest Debian/Ubuntu LTS, which in this case would be v14? If using v17, the question would be "why not upgrade to v18 when it is released" b/c there is no real reason to use v17 except that there is currently a brew.sh binary available for this version - but there is no real indicator of how long this will be supported for all platforms by their package manager, the llvm PPA only seem to support the latest 3 major version of llvm? This also wouldn't be a problem if clang didn't change its formatting rules through versioning :/
Personally, I am also really hesitant to install something globally through a package manager for something like linting code - everything concerning development should be scoped within a project and not attached to a global setup of a machine. But at the same time, a binary like this should be installable through a package manager, b/c otherwise it becomes a really tedious manual process.
As mentioned above, switching to pre-commit would make this complicated setup of getting the right clang format version for your platform a non-issue.
It would also not interfere with a global installation of clang/llvm, but instead keep it isolated for the supercollider repo folder - the version would also be manageable via code.
I think the current formatting setup is really hard and cumbersome for small changes - it has already taken many hours of my life that I would rather not have spent setting up the formatting and my main motivation is that other people don't have to go through this again :/
So my proposal would be to use v14 for now and then with another PR switch to pre-commit which can take care of obtaining the proper clang-format version for the current platform through PyPI wheels.
After this is done we can simply upgrade clang-format without having problems of supporting platforms again. And python 3 is already a dependency for the formatting process.
@capital-G Can you test with that version from brew or not? This would answer the main question. Then we can know if the version really matters or it's ok to be somewhat flexible.
@capital-G Can you test with that version from brew or not? This would answer the main question. Then we can know if the version really matters or it's ok to be somewhat flexible.
A bump to 17 would be resulting in a big patch w/o adjusting the rules prior
>git diff --stat
QtCollider/primitives/prim_misc.cpp | 2 +-
common/XenomaiLock.cpp | 2 +-
common/sc_popen.cpp | 2 +-
common/sc_popen.h | 2 +-
editors/sc-ide/widgets/code_editor/editor.cpp | 2 +-
editors/sc-ide/widgets/util/key_sequence_edit.hpp | 4 ++--
external_libraries/link | 0
include/plugin_interface/SC_InterfaceTable.h | 8 ++++----
include/plugin_interface/SC_Unit.h | 2 +-
lang/LangPrimSource/PyrPrimitive.cpp | 2 +-
lang/LangSource/AdvancingAllocPool.cpp | 2 +-
lang/LangSource/PyrInterpreter3.cpp | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------------------
lang/LangSource/PyrLexer.cpp | 14 +++++++-------
lang/LangSource/PyrObject.cpp | 4 +++-
lang/LangSource/SimpleStack.cpp | 2 +-
platform/windows/compat_stuff/getopt/getopt.c | 10 +++++-----
server/plugins/BeatTrack.cpp | 32 ++++++++++++++++----------------
server/plugins/Convolution.cpp | 2 +-
server/plugins/DelayUGens.cpp | 6 +++---
server/plugins/ML.h | 10 +++++-----
server/plugins/PV_ThirdParty.cpp | 2 +-
server/plugins/PV_UGens.cpp | 2 +-
server/plugins/UnaryOpUGens.cpp | 8 ++++----
server/scsynth/SC_CoreAudio.cpp | 4 ++--
server/supernova/server/memory_pool.hpp | 4 +++-
server/supernova/utilities/static_allocator.hpp | 4 +++-
server/supernova/utilities/tl_allocator.hpp | 4 +++-
server/supernova/utilities/utils.hpp | 4 +++-
tools/clang-format.py | 4 ++--
29 files changed, 127 insertions(+), 117 deletions(-)
100 lines of diff is very small all things considered, and the most common change seems to be adding a space after // which people should do anyway in new code. So I think we could say clang-format 14+ is okay. :) People should not be formatting entire files/repo but rather just the diff.
100 lines of diff is very small all things considered, and the most common change seems to be adding a space after
//which people should do anyway in new code. So I think we could say clang-format 14+ is okay. :) People should not be formatting entire files/repo but rather just the diff.
I think if we decide to use a formatter it has to be deterministic, otherwise we will have arguments over formatting again and it can break CI, so we have the worst of both worlds. :D Especially something like non-spaces after // is something that may occur more often that one think in everyday commits?
Also it is unclear how clang-format-18 would behave.
I think if we decide to use a formatter it has to be deterministic, otherwise we will have arguments over formatting again and it can break CI, so we have the worst of both worlds. :D
Perhaps you misunderstood? I am just saying to document, that we use clang-14 in CI and future versions will probably work fine, but use at your own risk. That will not result in arguments (CI linter has final say) or breaking CI (we only merge things which pass linting).
Especially something like non-spaces after // is something that may occur more often that one think in everyday commits?
If someone's //<text> gets reformatted to // <text> that is great because it still passes clang-format14 and then I don't have to ask for it in review :)
Also it is unclear how clang-format-18 would behave.
Can also just say 14-17 instead... anyway it is not a big deal either way, just a bit of extra flexibility that I thought would be nice to offer people, and thought you would be in favor of it since you seem to have a big issue with restricting the version so much.
Perhaps you misunderstood? I am just saying to document, that we use clang-14 in CI and future versions will probably work fine, but use at your own risk. That will not result in arguments (CI linter has final say) or breaking CI (we only merge things which pass linting).
I think this would lead to arguments as someone formatting with clang-format-14 would get different results than clang-format-17 - ofc there is the CI which would be the judge in this case, but then we could already say, please use clang-format-14 from the beginning.
If someone's
//<text>gets reformatted to// <text>that is great because it still passes clang-format14 and then I don't have to ask for it in review :)
Yeah, I'll totaly agree that since we have the linter we should stick to it :)
Can also just say 14-17 instead... anyway it is not a big deal either way, just a bit of extra flexibility that I thought would be nice to offer people, and thought you would be in favor of it since you seem to have a big issue with restricting the version so much.
Well, I think there is a third way here by providing an easy way to setup clang-format in its proper version by a package manager such as pre-commit. This would allow for determinstic formatting while also having a very easy workflow for contributors.
I did a small sketch here https://github.com/supercollider/supercollider/commit/25f59c68771872541e9f35b0e77ea98197140ea1 which simply adds a single file to the PR
❯ pre-commit install
// setups [email protected] from PyPI in a venv
❯ pre-commit run
Check Yaml...............................................................Passed
Check JSON...........................................(no files to check)Skipped
Check for merge conflicts................................................Passed
Check for added large files..............................................Passed
Check C++ formatting.....................................................Passed
The nice thing, I don't have to "taint" my system
❯ which clang-format
clang-format not found
Everytime one creates a commit, the pre-commit hooks will be checked if they are up-to-date and then be executed.
If clang-format is bumped via .pre-commit.config.yml, the user doesn't have to do anything, which even allows for working between branches using clang-format-14 and clang-format-18.
If the formatter complains, it will stage the formatting changes which needs to be applied in order to pass the formatter.
For example if I'd like to commit
pre-commit would fail and would stage the following changes which would make the formatter pass.
I think this would be really beneficial for every contributor as now linting becomes completely automatic and already setup. But it was suggested to split both actions.
Oh - and we should check why https://github.com/supercollider/supercollider/pull/6096 has less LOC affected than this, although it uses the same formatting rules. Maybe I excluded too much files in #6096?
I think this would be really beneficial for every contributor as now linting becomes completely automatic and already setup.
Please let's keep this review conversation to just the clang-format version. It is a lot to consider on its own.
Oh - and we should check why https://github.com/supercollider/supercollider/pull/6096 has less LOC affected than this, although it uses the same formatting rules. Maybe I excluded too much files in https://github.com/supercollider/supercollider/pull/6096?
Yes that would be good to know. I took a glance at it but couldn't spot a reason. It will probably be awhile before I can look into it further, if you can figure out why that happens I'd appreciate it.
I compared the changes of both PRs and the differences seems to be
- Multiple times some small statements got inlined, for example
diff --git a/lang/LangPrimSource/OSCData.cpp b/lang/LangPrimSource/OSCData.cpp
index a8743ef17..14f30efcd 100644
--- a/lang/LangPrimSource/OSCData.cpp
+++ b/lang/LangPrimSource/OSCData.cpp
@@ -777,9 +777,7 @@ void init_OSC(int port) {
try {
gUDPport.reset(new InPort::UDP(port, HandlerType::OSC));
- } catch (std::exception const& e) {
- postfl("No networking: %s", e.what());
- }
+ } catch (std::exception const& e) { postfl("No networking: %s", e.what()); }
}
int prOpenOSCUDPPort(VMGlobals* g, int numArgsPushed);
- In the old PR I ignored
platform/windows/compat_stuff/getopt/getopt.cwhich new adds a change on 262 loc
I don't understand why, under the same ruleset, the formatting differs?
In the old PR I used version 14.0.1, for this commit I used version 14.0.6.
I reset to "before the formatting" and ran formatall with 14.0.1 but it now also inlined the changes - I really don't know whats the cause for the diff, but at least the formatting within branch seems deterministic for version 14.x.y.
I reset to "before the formatting" and ran formatall with 14.0.1 but it now also inlined the changes - I really don't know whats the cause for the diff, but at least the formatting within branch seems deterministic for version 14.x.y.
Just to make sure I understand, the reason for the difference was that you used different versions of clang-format between the PRs?
I reset to "before the formatting" and ran formatall with 14.0.1 but it now also inlined the changes - I really don't know whats the cause for the diff, but at least the formatting within branch seems deterministic for version 14.x.y.
Just to make sure I understand, the reason for the difference was that you used different versions of clang-format between the PRs?
Sorry for the confusion - I checked if non-matching patch versions could be the cause for this but this does not seem to be the case. So I don't have a clue where the inline originates from.
Just ran into this today when the gh tests failed but I had formatted on my machine using clang-format-16. Took a long time to find out why...
Isn't bumping the version going to cause the same issue later on? Perhaps reviewers could just use their judgement and recommend people format using clang, then, periodically (say once every few months), someone could run clang-format (with what ever version) across the entire code base? This would remove a dependency from the project (or at least the CI).
Isn't bumping the version going to cause the same issue later on? Perhaps reviewers could just use their judgement and recommend people format using clang, then, periodically (say once every few months), someone could run clang-format (with what ever version) across the entire code base? This would remove a dependency from the project (or at least the CI).
Makes it impossible to follow history and you will run into tons of merge conflicts.
Any news on this?
There is no easy way to install clang-format-8 on arch based systems, and building llvm just for a formatter is a big ask.
It is annoying. @capital-G has looked into this, I think, but he is on travels for some weeks.
Any news on this?
@JordanHendersonMusic In my view this PR is ready to be merged and waits for approval. As long as we don't have any open PRs that touch the adjusted areas of code by the new rules we should be good to go. I couldn't reduce the impact of the update any further and the urgency of the update seems to grow.
Is this change necessary if the precommit is done instead? That seems like a better solution?
@JordanHendersonMusic This PR was created to separate the two concerns as requested. This PR prepares the codebase to pass the clang-format-14 formatter so we can deprecate clang-format-8 as formatter so other people can format the c++ code more easily again.
Due to the urgency of the clang format update and an option but non-necessity to use of pre-commit to take care of formatting-dependencies and commit-sanitizing (which I would also prefer), I think it's best to go ahead with this PR and afterwards create a PR which introduces pre-commit for this.