install clang-format using pip, don't expect users to have the correct one
Turns out one can install clang-format through pip, just like the python formatter black. This is surely a better way, as it doesn't require users to provide the correct version themselves, and is portable. I modified format.sh to install a specific version of clang-format now, and cleaned up and added additional checks for both format.sh and format_python.sh.
Usage of the scripts remains as-is. The option to override which clang-format exec to use via CLANG_FORMAT_CMD variable remains.
I successfully ran both formatting scripts on cosma after loading the python/3.10.1 module. It'll likely work with other python3 modules as well, but I didn't check.
I also switched to clang-format 16, because I can. It introduces very minimal changes (for commented out macros, it adds a space between the comment and the beginning of the macro). There's like 10-20 instances in 4-5 files. You might wanna run the formatting scripts immediately after merging this MR.
Merge request reports
Activity
assigned to @matthieu
requested review from @pdraper
OK, so let's make sure we know what this is. It is downloading LLVM and building the clang-format command (must be so, only way to comprehend a language this well is to use the lexical output of an actual compiler). So, yes, convenient, but the limitations are it is still an LLVM build, albeit of just the parts needed for clang-format, and will not work on systems without internet access, like COSMA.
Tempted to say why not and fix up Jenkins as needed...
Not 100% sure how Jenkins works in specific, but there is easy workarounds.
Option 1: If jenkins uses pre-existing directories, then pre-install the formatter.
Option 2: make sure CLANG_FORMAT_CMD variable is set while running the job. That bypasses the build of a specific clang-format through pip.
We can adapt the required version of clang-format in the script to whatever is available on cosma.
Edited by Mladen Ivkovic
added 1 commit
- d2bd2575 - only check for pip/python if we're installing clang via pip
@jborrow you usually have strong opinions on best python practices. Any thoughts?
I note that on the latest ubuntu,
pipis blocked as they favour the usage of their package manager. You can bypass it though. Might be something coming from debian and hence percolating into other distributions.Is this the kind of warning/error you're seeing? https://askubuntu.com/questions/1465218/pip-error-on-ubuntu-externally-managed-environment-%C3%97-this-environment-is-extern
Because if it is, we're fine. We're installing black and clang-format in a venv.
It does seem kind of weird to me to install a C compiler via pip. Do we know what flags it is compiled with? But on the other hand this does appear to be an 'official' LLVM product.
At the end of the day it comes down to a matter of taste; are we comfortable using pip (which, all said, is a pretty dodgy package manager) for situations where we can reasonably get away with something else? I would probably say 'no', users should install clang-format themselves.
Users can always pip install clang-format for themselves (I will probably do this on random systems in the future), but I am not sure we should maintain this as our 'official' channel.
Well, we're already using pip for the python formatting. If we wanna be consequent, we should remove that and ask users to install a specific version of black.
The package doesn't provide you with a full compiler. It looks like they just wrapped a precompiled portable binary in python. It installs in seconds. I'm not sure why the compilation flags for a formatting tool would be of interest to us, as long as it doesn't deliver faulty results? And since it's neatly packed up into a venv, chances of interfering with your system should be low.
My thoughts were that this would be a change which would give us one less thing to worry about in the future. Asking users to instal clang-format themselves is all fun and games until systems decide not to provide specific packages any longer and then users can either go fry uncle chester's kebab themselves or compile LLVM from scratch. Which is not a fun exercise, even with automatized tools and managers like spack.
Additionally, with the newly added version checks for the scripts, all we need to do to switch between formatter versions is to change them in the master branch. If a wrong version is detected, the format scripts will first complain, and then ask you to purge the venv so they can install the correct versions. So my hopes were that this leads to less headaches down the road.
However, it looks to me like I'll be outvoted on this matter. If that's the case, let's just close this MR and be done with it.
Well the major difference there is that black is a python library for formatting python files. That makes sense to be distributed as part of the Package Installer for Python :).
I do like the fact that this allows us to pin a version of clang-format (by installing a specific version of the python package).
My major concern really comes from the fact that the OS will likely use whatever clang-format is on the system, so when people use format-on-save features they may be inconsistent with the formatting that we get from running format.sh and cause weird collisions. That's why I think it's nice if we tell users that they can install clang-format with pip, and that they should use a specific version.
My major concern really comes from the fact that the OS will likely use whatever clang-format is on the system, so when people use format-on-save features they may be inconsistent with the formatting that we get from running format.sh and cause weird collisions.
That's gonna be a problem with our format.sh anyway. As you said, different versions of clang-format may deliver slightly different results. We already have a specific (major) version of clang-format specified in format.sh on current master. Meaning that unless users have the same version as we do for jenkins, it's gonna keep failing the automatic tests. Even worse, it'll keep oscillating between the formatting of that one user and everybody else running the format script.
So if a user installs the wrong version of clang-format manually, or uses the wrong one with their IDEs or whatever, it's not gonna end up great. Having pip install a specific encapsulated version by default eliminates that.
My point is that we want users to have a consistent version of clang-format inside and outside of format.sh. That means that we can prompt them to install the 'correct' version, and use that everywhere. The nightmare scenario here is that they have two different versions of format.sh installed on their system. Most people (myself included) just set up the 'correct' version of clang-format in their IDE and then never actually run format.sh.
mentioned in merge request !1962 (merged)
