Dealii: Switch to clang-format

[bangerth]

In #4471, about a year ago, we argued about whether we should upgrade to astyle 3.01. We could now upgrade to 3.1.

Advantages: Better indentations for C++11.
Disadvantages: Lots of pain because every single one of us needs to update, and we also need to update the CI machine.

Opinions?

[davydden]

Advantages: Better indentations for C++11.

that's big enough

Disadvantages: Lots of pain because every single one of us needs to update, and we also need to update the CI machine.

it's not that much pain, IMO. For me it's just spack install astyle 😄 But seriously, if we move to a different indentation, it's not a big deal, as long as everyone agrees. To that end le't do a poll:

👍 move to [email protected]
🎉 move to clang-format
👎 stick with [email protected]

@dealii/dealii ping.

[bangerth]

The pain isn't only on us, but on those several dozen people who contribute over time and who would now have to install something new. I'd like it a lot if we at least chose something that's easy to install.

[davydden]

I'd like it a lot if we at least chose something that's easy to install.

i think the two options (astyle and llvm) are comparable in that astyle is easy to install whereas llvm is provided by many linux distros and it's also easy to install on mac with Spack.

[kronbichler]

I have never tried clang-format, but the major question is on the stability of output between different releases of llvm. We must assume that not everyone is able to access a particular version of llvm. If you don't have the right version and we have to force a particular version, installing llvm takes a little while and is way more tedious (also with spack) as compared to astyle which is a single executable.

[davydden]

@kronbichler that's fair point, i don't how stable clang-format is between different versions. And the installation certainly less robust than a simple astyle.

[bangerth]

I think someone will need to try this out with different versions of clang-format. We can't expect anyone to install a whole compiler suite by themselves, let alone a particular version.

[tamiko]

While I generally believe that clang-format is the better tool (and as far as I am concerned, the output is more or less invariant between versions), the fact that not everyone can easily install it is the major problem with it.

I am in favor of migrating to astyle-3.1. It fixes quite a number of pet peeves I have with the current version we support.

EDIT[DD]:Full diff

[luca-heltai]

@tamiko , is astyle 3.1 a projection? astyle(astyle(x)) = astyle(x)?

I recall that this was not the case with 3.0.1, which is why we did not migrate there...

[bangerth]

Oh, you're right. I just tried this out and it toggles between these two forms:

diff --git a/include/deal.II/base/mpi.h b/include/deal.II/base/mpi.h
index 78db9b5..d981e95 100644
--- a/include/deal.II/base/mpi.h
+++ b/include/deal.II/base/mpi.h
@@ -530,7 +530,7 @@ namespace Utilities
        * <code>main()</code>.
        */
       MPI_InitFinalize (int    &argc,
-                        char ** &argv,
+                        char** &argv,
                         const unsigned int max_num_threads = numbers::invalid_unsigned_int);

       /**
diff --git a/source/base/mpi.cc b/source/base/mpi.cc
index 1074e93..f62445a 100644
--- a/source/base/mpi.cc
+++ b/source/base/mpi.cc
@@ -384,7 +384,7 @@ namespace Utilities


     MPI_InitFinalize::MPI_InitFinalize (int    &argc,
-                                        char ** &argv,
+                                        char** &argv,
                                         const unsigned int max_num_threads)
     {
       static bool constructor_has_already_run = false;

That's the only place I can find, so maybe we can work around this somehow. But it for sure is annoying!

[bangerth]

I can work around this with parentheses.

[davydden]

@tamiko

the fact that not everyone can easily install it is the major problem with it.

apt-get install clang-format-3.9 sounds easy enough for me.

We also can do this with Github bot!
As I said, mac users are ok as well, especially given that @luca-heltai already builds dmg with Spack, so we can simply ship installed llvm there as well.

[Rombur]

i don't how stable clang-format is between different versions

It is not stable. There are very small differences between the versions. We use clang-format all the time and there are usually a couple of lines that are different with each version of clang-format.

For people on ubuntu/debian, they can just grab the version of llvm from here http://apt.llvm.org/ and install the version they want. So it's pretty easy to install if you have sudo otherwise you need spack.

[bangerth]

On 05/15/2018 08:12 PM, Denis Davydov wrote:

|apt-get install clang-format-3.9| sounds easy enough for me.

This is only possible for some operating systems (and linux variants). Not
even all past or future versions of Ubuntu will have this specific version of
clang available in their repositories. For example, older linux installations
may not be up to version 3.9, and newer installations may be beyond 3.9.

There is also the issue that you need root privileges. Unless someone can
demonstrate that it is easy to install clang-format as a user, I'm not in
favor of this solution...

[davydden]

p.s. @tamiko do you mind if I delete your full patch? It requires whole lot of scrolling to follow the discussion.

[drwells]

I think clang-format is likely to win this poll, but I would really prefer not to use an indentation tool that produces slightly different output on different versions.

[Rombur]

@drwells but that's not different from astyle. If you change the version of astyle, the output will be different.

[masterleinad]

This is only possible for some operating systems (and linux variants). Not even all past or future versions of Ubuntu will have this specific version of clang available in their repositories. For example, older linux installations may not be up to version 3.9, and newer installations may be beyond 3.9.

Currently, we have:

Package clang-format-3.9
    trusty (14.04LTS) (devel): Tool to format C/C++/Obj-C code [universe]
    1:3.9.1-4ubuntu3~14.04.3 [security]: amd64 i386
    trusty-updates (devel): Tool to format C/C++/Obj-C code [universe]
    1:3.9.1-4ubuntu3~14.04.3: amd64 arm64 armhf i386 powerpc ppc64el
    xenial (16.04LTS) (devel): Tool to format C/C++/Obj-C code [universe]
    1:3.9.1-4ubuntu3~16.04.2 [security]: amd64 i386
    xenial-updates (devel): Tool to format C/C++/Obj-C code [universe]
    1:3.9.1-4ubuntu3~16.04.2: amd64 arm64 armhf i386 powerpc ppc64el s390x
    artful (17.10) (devel): Tool to format C/C++/Obj-C code [universe]
    1:3.9.1-17ubuntu1: amd64 arm64 armhf i386 ppc64el s390x
    bionic (18.04LTS) (devel): Tool to format C/C++/Obj-C code [universe]
    1:3.9.1-19ubuntu1: amd64 arm64 armhf i386 ppc64el s390x
    cosmic (devel): Tool to format C/C++/Obj-C code [universe]
    1:3.9.1-19ubuntu2: amd64 arm64 armhf i386 ppc64el s390x

[masterleinad]

And there are prebuilt binaries for llvm-3.9 for:

Clang for Mac OS X (.sig)
Clang for FreeBSD10 AMD64 (.sig)
Clang for FreeBSD10 i386 (.sig)
Clang for AArch64 Linux (.sig)
Clang for armv7a Linux (.sig)
Clang for Fedora23 i686 Linux (.sig)
Clang for Fedora23 x86_64 Linux (.sig)
Clang for OpenSuSE 13.2 i586 Linux (.sig)
Clang for OpenSuSE 13.2 x86_64 Linux (.sig)
Clang for x86_64 Ubuntu 14.04 (.sig)
Clang for x86_64 Ubuntu 16.04 (.sig)
Clang for x86_64 Debian 8 (.sig)
Clang for MIPS (.sig)
Clang for MIPSel (.sig)
Clang for Windows (32-bit) (.sig)
Clang for Windows (64-bit) (.sig)

[drwells]

@Rombur This is a good point. I disabled updates for astyle on my machines for this reason. I still think its easier to keep an outdated version of astyle than an outdated version of all of LLVM so I am keeping my vote where it is.

[tjhei]

Is astyle 3.1 a projection? astyle(astyle(x)) = astyle(x)?
I can work around this with parentheses.

This is a big issue if we continue to test the formatting using CI. Somebody introduces a new piece of code that triggers this, which will break any follow-up PR. We would need our indent script to run twice and check that not differences show up and fail otherwise?!

There is also the issue that you need root privileges. Unless someone can
demonstrate that it is easy to install clang-format as a user, I'm not in
favor of this solution...

I agree, this is somewhat annoying.

I still think its easier to keep an outdated version of astyle than an outdated version of all of LLVM

Agreed, so that is my vote as well. Too bad. ;-(

[bangerth]

I'm trying to reconcile the arguments. I believe that we are all in agreement of the following:

• clang-format is a cool technology: it understands the code because it's based on a compiler, and that definitely speaks to the nerd in me (and maybe many of you)
• astyle is clearly substantially easier to install across platforms and at a defined version

What I think I don't understand is what the technical arguments for clang-format are. Yes yes, as nerds we are attracted to cool solutions, but is it actually better? In other words, what do we gain for the pain? Are there things that clang-format can fundamentally do or do better than what astyle can?

[davydden]

@bangerth

Unless someone can
demonstrate that it is easy to install clang-format as a user, I'm not in
favor of this solution...

$ spack install llvm
<skipped>
==> No patches needed for llvm
==> Building llvm [CMakePackage]
==> Executing phase: 'cmake'
==> Executing phase: 'build'
==> Executing phase: 'install'
==> Successfully installed llvm
  Fetch: 0.87s.  Build: 1h 19m 50.21s.  Total: 1h 19m 51.08s.
[+] /Users/davydden/spack/opt/spack/darwin-highsierra-x86_64/clang-9.1.0-apple/llvm-6.0.0-ip6xtfnpg2x45cdprup7fcom4mbyd72x
$ spack load llvm
$ clang-format --version
clang-format version 6.0.0 (tags/RELEASE_600/final)

Also downloading binaries should be easy.

[tamiko]

clang-format operates in the following way: It first uses the clang lexer to build an abstract syntax tree of the source code and afterwards pretty-prints it. Consequently, the indenting results are very stable, very uniform (almost independent of input) and (ideally) should not vary between versions.

The downside is - as already discussed - that we need to compile a full compiler toolchain for it, or provide some downloadable, self-contained binaries. Further, it does not have that many configuration options.

I will play around with clang-format a bit more tomorrow. In particular I am interested in

• getting a configuration that produces (nearly) identical output for clang 4/5/6 (these are the versions that are available in Gentoo).
• include a custom perl script to correctly format our comments

If these two things pan out, we could for example provide a small installation script that simply downloads a binary, statically linked clang-format executable, say for version 6.

[tjhei]

statically linked clang-format executable

That would be preferable over compiling llvm from scratch.

[tamiko]

Here is an excerpt of what clang-format (with the current .clang-format configuration file) would do to our sources: https://gist.github.com/tamiko/4755d9007eea0ac7aeba88b76194bd48

[bangerth]

The gist seems to be served by a system that is blocked from China :-)

But in any case, I have not heard what makes clang-format better than astyle. Given that 4 of the 5 votes for clang-format came from Europe, let's give them some time to wake up ;-)

[masterleinad]

For me, the proper support of C++11 (and newer standards) syntax is the most compelling argument.

4936 and #6550 would be good examples. It is just annoying that we are currently forced to find workarounds for getting a sensible indentation from astyle.

We also have some PRs (#4936, #4555) where we mentioned that clang-format could have done the job automatically. Some improvements can be found in #4476 and #1677 as well.

Another argument might be that we can automatically enforce the maximum lines of characters in a line, or sort header files inclusions.

Also, there is native support in a lot of IDEs like Eclipse, Visual Studio Code, Clion, QtCreator and Netbeans. Furthermore, plugins for command line editors like vim and emacs are available.
clang-format will likely survive the next years whereas this might not be true for astyle.

With greetings from Erlangen,

J-P, Denis, and Daniel

[tamiko]

But in any case, I have not heard what makes clang-format better
than astyle. Given that 4 of the 5 votes for clang-format came from
Europe, let's give them some time to wake up ;-)

To make it short clang-format is an OCD dream. And the only conclusion
that I can draw from your remark is that you never tried it :-P

(I force all students to have run clang-format on their source code
before I see it.)

[bangerth]

I guess I regret having opened this issue :-(

To make it short clang-format is an OCD dream. And the only conclusion
that I can draw from your remark is that you never tried it :-P

That is true.

I rarely have the distinct feeling that we're making a fundamental mistake
with a decision, but this one causes that for me. All of the arguments I read
are of the form "it would be nice to have this", but not of the "this enables
us to do something fundamentally different". Of course, from an OCD
perspective, I am as excited as everyone else. I'm just terribly concerned
that we erect more hurdles for would-be contributors -- that's something I
really worry about.

To give just one example of why it's not easy to install clang-format, from
someone who should really know how to do this: I foolishly upgraded my laptop
with Ubuntu 17.04 last year. But that's not a long-term support release, and
its support has run out. What that means in practice is that I can't do
'apt-get install clang-format' any more, or 'apt-get install spack' for that
matter -- the repository from which apt-get draws simply doesn't exist any
more. I know that's silly, and I'm sure any number of you will now come to my
rescue and show me that I can work around this by doing X or Y, but that's not
the point. [The correct work around is of course to install an OS that does
not expose me to security risks because it's not updated any more.] Another
example is on clusters where you don't have root access, and also don't have a
lot of space in permanent directories to install something as big as a whole
compiler. The point is that if we require contributors to install a package
that's either (i) not trivial to install, or (ii) for which the workaround
involves a four step sequence of commands that's impossible to remember, then
we'll lose a certain percentage of these contributors because they think that
it's just silly to have to jump through so many hoops.

I know full well that clang-format is the technically superior solution. So is
using C++17 over C++11. But we make compromises for these sorts of things so
that we reach a maximal number of users if we think that we can live with the
drawbacks. If the compromise is that we don't enforce sorting of header files
as part of the CI process, or limiting line length, then we can still do this
periodically whenever one of us feels the urge -- essentially all of the PRs
that the Erlangen crew pointed out today were in fact someone having an itch
and running a Perl script, or a different version of astyle. I have no problem
with it if an individual developer wants to occasionally run clang-format and
submit a one-off PR for that; or wants to use the clang-plugin in their
favorite editor to do this or that; etc.

What I'm arguing for is a solution that (i) does not drive anyone away because
it really is difficult to install, all the protestations notwithstanding,
for anyone who is not as technically skilled as all of you, and (ii) perfectly
good enough for what we want to do, even if it's not perfect.

[davydden]

@bangerth you keep repeating that it's difficult to install. But it really is not if you don't do it by hand. Either download binaries or use Spack which does not require root and works on a cluster (if you want to develop on a cluster). I really don't think it's a barrier for anyone to contribute, especially since clang-format is available on most Linux distros, and so are binaries...

[bangerth]

On 05/16/2018 08:50 PM, Denis Davydov wrote:

@bangerth https://github.com/bangerth you keep repeating that it's difficult
to install. But it really is not if you don't do it by hand. Either download
binaries or use Spack which does not require root and works on a cluster if
you want to develop on one.

But then you didn't read why I, for example, can't do either of these two
options on my laptop here; and why it's difficult on a cluster if your
permanent file system has a quota (as they all seem to have).

[davydden]

you didn't read why I, for example, can't do either of these two
options on my laptop here

sorry, i dont' see where you try to use Spack in previous replies and how does not work... Maybe I missed it. Can you elaborate how it does not work?

why it's difficult on a cluster if your permanent file system has a quota

I use it on cluster to build deal.II suite, Spack is clever enough to delete build folders after the sucesfull build. I have not see anyone complaining that you can't use it because of quota.

[drwells]

I suppose that I have not tried to install clang-format by hand (or with spack). I should try to do this to be fair. Where are the binary downloads?

[davydden]

Where are the binary downloads?

binary of Spack, there is none, just git checkout, it's written in Python. see https://github.com/dealii/dealii/wiki/deal.II-in-Spack

[drwells]

sorry, I meant binaries for clang-format.

[tamiko]

@drwells http://releases.llvm.org/download.html

[tamiko]

@bangerth

To give just one example of why it's not easy to install clang-format, from someone who should really know how to do this: I foolishly upgraded my laptop with Ubuntu 17.04 last year

But that prevents you from installing cmake, ninja, make, doxygen, astyle, etc. for the very same reason.

We already provide setup_astyle.sh and download_codecov_bash.sh in contrib/utitilities for exactly the reason you mentioned. If we were to provide a download_clang_format.sh that simply downloads a statically built binary version, then I don't think your installation argument will work.

[drwells]

I will install clang-format on my trusty RHEL7 workstation and report back on how difficult it is.

[bangerth]

On 05/16/2018 09:09 PM, David Wells wrote:

I install |clang-format| on my trusty RHEL7 workstation and report back on how
difficult it is.

While you're there, can you also report back (i) how much disk space that
permanently uses up, (ii) how long it takes clang-format to indent our entire
code base?

[bangerth]

On 05/16/2018 09:07 PM, Matthias Maier wrote:

If we were to provide a |download_clang_format.sh| that simply downloads a
statically built binary version, then I don't think your installation argument
will work.

Yes, that will work. We'd just have to make it work for linux, mac, and windows.

[masterleinad]

(ii) how long it takes clang-format to indent our entire code base?

Without running in parallel, I just needed two minutes.

[masterleinad]

(i) how much disk space that permanently uses up,

I only installed clang-format-5 instead of the whole llvm suite and it takes less than 2MB:

$ du -sh /usr/lib/llvm-5.0/
1,6M    /usr/lib/llvm-5.0/

[tamiko]

Small status update:

• @Rombur is right that major versions also come with small changes in clang-format behavior (but it is much less than astyle)
• I have created a small static binary build that can be downloaded here (sig) (fully static, thus 11MB unpacked, 3.7MB tarball)

I will create a pull request later today that includes two scripts compile_clang-format and setup_clang-format. The former compiles a static clang-format tool, the latter one will download above binary.

[drwells]

All right, here it goes: full documentation of my attempts to get clang-format
working.

I am working on an RHEL7 box without root access: this should be similar to what
a user would have on their workstation or cluster.

prebuilt binaries

I'll do a few unusual things just to see what a user might encounter.

Fedora version

The binaries for fedora are too new and don't work with my older system.

RHEL version

these are ancient (e.g., GCC 4.2 compatibility) and are not worth investigating.

conclusion

I'd be very happy if there was an easy way to download binaries for RHEL to dump
in my home directory but none are available.

from spack

Running

spack install clang-format
doesn't work, which is annoying.

(10:33 AM) (install LLVM attempt 1) It looks like I have to install the entire
GNU tool-chain; I am recompiling m4 and bison.

(10:34 AM) Does spack respect MAKEFLAGS=-j16? It would be nice if that worked.

(10:45 AM) spack claims that I don't have any space left on my device, but I
have 100 GB free in my home directory. This is probably a bug. It looks like
spack is respecting MAKEFLAGS=-j16 at least.

(10:50 AM) (install LLVM attempt 2) This has to be a bug with spack: spack
crashed since I ran out of temporary space but left about a gigabyte of stuff in
/tmp/$HOME/spack-stage/: not good. I nuked the directory and started again.

(10:53 AM) (install LLVM attempt 3) Spack is still crashing when it fills up my
/tmp/ directory: it managed to copy another gigabyte worth of stuff when
compiling binutils. I guess spack doesn't use -pipe? I will use export TMPDIR=$HOME/temp/ in an attempt to fix this.

(11:05 AM) (install LLVM attempt 3) I am currently compiling python.

(11:11 AM) (install LLVM attempt 3) I am finally building LLVM. The temporary
directory needs at least 2.2 GB to compile it.

(11:25 AM) (install LLVM attempt 3) done! It works!

Compiling this took about an hour on my workstation and required at least two
gigabytes of disk space. It also crashed due to the way my /tmp/ directory is
set up. The SPACK_ROOT directory currently needs 5.2 GB as well.

compiling astyle

Using the provided shell script takes about two seconds and 2 MB of disk space.

From Matthias' binary

This works perfectly on the first try. Very nice!

conclusions

Asking users to compile all of LLVM is a lot, especially compared to astyle. I encountered a bunch of problems trying to get spack to work correctly that users might not know how to fix.

The static binary @tamiko put together is a reasonable solution to all of our problems. Its really incredible that we can compress this thing down into 11 MB: I did not think that would be possible.

[davydden]

@drwells

from spack

few comments from my side:

• Spack will use all cores unless a package declares explicitly to build in serial. So it does build everything in parallel spack install --help tells you -j JOBS, --jobs JOBS explicitely set number of make jobs. default is #cpus.
• it's true that by default no information of present tools (m4, binutils, blas, lapack, etc) is taken into account (that's why it works on clusters). On my workstation it's usually simple to let it install needed tools and be done. Sysadmins can setup all external packages and skip the build of unnecessary things, but for users it usually does not worth the hassle.
• ~1 hour is roughly how much it takes me to build llvm as well.

[drwells]

I misread the spack documentation in my haste and ran spack --help install instead of spack help install. You're right, of course.

@tamiko claims that there is a simpler way to compile a standalone clang-format (if I understood correctly, without compiling all of LLVM) but he is on a plane now. I'll wait until he comes back. I am pretty happy with the 11 MB static executable approach.

[drwells]

@bangerth It takes about 12 seconds to run make -j8 indent with clang-format and about 4 seconds to run make -j8 indent with astyle.

[drwells]

Since it is approximately as easy to install clang-format as astyle (and we can pin a version, but it sounds like that should not be as irritating as it was with astyle) I no longer have any substantive complaints against using it. I suggest we make a decision soon so we can start fighting about the settings in .clang-format next :smiley:

[davydden]

@dealii/dealii now that the clang-format config file is out of the way, what are the next steps in the switch? Supposedly, update CI and apply the style?

[masterleinad]

@dealii/dealii now that the clang-format config file is out of the way, what are the next steps in the switch? Supposedly, update CI and apply the style?

Yes, I already have something in https://github.com/masterleinad/dealii/tree/update_clang_format_style_file_backup I can just polish up a bit.

[luca-heltai]

Is there already an indent_using_clang_format script somewhere, that I can drop into travis?

[davydden]

@luca-heltai

Is there already an indent_using_clang_format script somewhere, that I can drop into travis?

it's here https://github.com/dealii/dealii/pull/6667 but we probably first need to merge that PR