Fix broken builds and missing install targets

[csnover]

This patch rewrites the existing CMake files so that they export targets and install headers in a way that allows the application and libraries to be used idiomatically.

Included third-party code now builds object libraries that are embedded in the final archives and public headers from those third-party libraries are installed in subdirectories of the embedding libraries so they do not conflict with any other system installation.

This patch also fixes compiling the drawdance and dpgles2 targets, the emscripten cross-compilation, and building with SDL versions below 2.0.20, which were all broken.

Emitted output during build generation is revised to provide information about which features are enabled.

Finally, this commit removes clang-format from CMake, since this cannot work with all generators according to
https://discourse.cmake.org/t/clang-format-integration/3358/4 and should go into a git pre-commit hook instead or integrated in the IDE.

[askmeaboutlo0m]

Very cool! I like the use of generator expressions, they make the whole thing a lot less fiddly. I'm very surprised that the standalone and web version are still running at all, since I've been focusing on Drawpile exclusively, but that's sweet. Excluding saving and loading for now is a clean way to get the web stuff going again too.

Removing clang-format is also a good change. The error messages it gave in that way are awful and it has to be kept in sync. Better to leave it to separate toolage, which I've been doing anyway.

And I can confirm that it builds on Linux (after some mangling related, but since you also managed to build it, I assume this is fixed already.)

Comments in order of importance:

Very against making bundling of libmypaint and parson optional, it just increases complexity and I see no gain in it. In particular, we patch libmypaint slightly to remove a goofy dependency on json-c (because we don't use that function anyway) and fix a smudging bug that's debilitating to any brush that uses smudging. (MyPaint doesn't notice this bug because they spam the brush with every pen motion event even while in the air, but that causes other annoying behavior, like the stabilizer applying before you push your pen down, which is bonkers.) Parson I'm less emotional about, it just increases the chance that someone will have an incompatible or broken version laying around.

The emscripten preset in scripts/cmake-presets is now busted and I'm not sure how to set it up otherwise, since the toolchain file is gone now. But the setup I had for it is pretty ancient at this point and still required a lot of hackery to get going at the time, so I assume my knowledge is just outdated.

Being able to install stuff is nice, and your dependency handling is way better than what cmake produced. Installing being mandatory is annoying though, can you think of a way to get it to load stuff from the build directory and reference the headers directly? Since in all cases I can think of it would just involve copying a bunch of files from the build directory to a directory right next to it. It may also lead to your editor sending you to edit a file in the install directory, which is not helpful since it will get clobbered without asking on the next install. Or maybe this is not worth worrying about at all, since we're gonna combine the two projects anyway and then installing an intermediate thing will be moot.

On gcc, clang-tidy doesn't work, since the build scripts now check if the compiler is a clang exclusively. Would probably make sense to add an elseif gcc in there and just look for a clang-tidy without any version number in it? On my Arch system, I also don't have any clang-tidy-15, the executable is just called clang-tidy. So maybe that should even be a general fallback for the clang case too.

Having read up on it: removing the license header and replacing it with an SPDX thingy is valid and good. Removing the copyright year, despite other projects doing so too, makes it an invalid copyright notice: it must contain "copyright" or some variation thereof, an author of some sort and the year of first publication. Solution 1: add the year back in. Solution 2: remove the copyright notice altogether, keep only the SPDX line. I'd prefer the latter.

Config.cmake.in and helper.cmake are missing license information. And there's currently nothing crediting you in the repo. Time for an AUTHORS file?

The use of directory-level configs made me have nightmarish flashbacks to old CMake versions. After a strong coffee and looking at them being used sensibly, I think this is okay though. Maybe put a small comment on the "disable warnings in this cursed place" property, since it's not too obvious how it works and the places it's used are kinda spread out, connected only by spooky action at a distance.

[csnover]

I'm very surprised that the standalone and web version are still running at all, since I've been focusing on Drawpile exclusively, but that's sweet.

Well, they compile now. macOS does not support OpenGL ES at all so that part definitely doesn’t work and I am not sure about the rest. :-)

And I can confirm that it builds on Linux (after some mangling related, but since you also managed to build it, I assume this is fixed already.)

Are you referring to building this repo or the Drawpile repo? This one should require no modifications but please verify in case I need to check against a different Linux distro.

Very against making bundling of libmypaint and parson optional, it just increases complexity and I see no gain in it. In particular, we patch libmypaint slightly to remove a goofy dependency on json-c (because we don't use that function anyway) and mypaint/libmypaint#186 that's debilitating to any brush that uses smudging. (MyPaint doesn't notice this bug because they spam the brush with every pen motion event even while in the air, but that causes other annoying behavior, like the stabilizer applying before you push your pen down, which is bonkers.) Parson I'm less emotional about, it just increases the chance that someone will have an incompatible or broken version laying around.

My apologies for the unclear remarks regarding this change. It is aligned with what you want. Since CMakeLists.txt doesn’t use find_package for these dependencies, it will never try to use a system version. The change makes these dependencies compile as object libraries, which means the built objects are added directly to the drawdance libraries and their public headers are installed into a drawdance-specific subdirectory. Previously, separate libmypaint/parson/etc. archives were created, and trying to install those would have caused conflicts if there were an existing system installation.

The emscripten preset in scripts/cmake-presets is now busted and I'm not sure how to set it up otherwise, since the toolchain file is gone now. But the setup I had for it is pretty ancient at this point and still required a lot of hackery to get going at the time, so I assume my knowledge is just outdated.

With the emsdk enabled the blessed way is to either run emcmake cmake instead of cmake, or else to add -DCMAKE_TOOLCHAIN_FILE=emsdk/upstream/emscripten/cmake/Modules/Platform/Emscripten.cmake when invoking cmake (emcmake mostly just automates away this unreasonably long path).

Being able to install stuff is nice, and your dependency handling is way better than what cmake produced. Installing being mandatory is annoying though, can you think of a way to get it to load stuff from the build directory and reference the headers directly?

I know that it is theoretically possible to just use add_subdirectory to include a CMake project into another project, but I’m not sure if any additional support code is needed or not to support this case. (It’s CMake, so… probably? :-() When working, it generates targets such that target_link_library ought to Just Work and use the correct interface include directory (IIUC this is the distinction between BUILD_INTERFACE and INSTALL_INTERFACE).

Generally in such cases if it doesn’t immediately work I’ll just use some script to run cmake --build <build_dir> && cmake --install <build_dir> instead, and then will use -DCMAKE_INSTALL_PREFIX=<build_dir>/install or whatever to shove it into the build tree, because life is way too short to try to make sense of CMake.

On gcc, clang-tidy doesn't work, since the build scripts now check if the compiler is a clang exclusively. Would probably make sense to add an elseif gcc in there and just look for a clang-tidy without any version number in it? On my Arch system, I also don't have any clang-tidy-15, the executable is just called clang-tidy. So maybe that should even be a general fallback for the clang case too.

It should work so I will double-check this. It is always possible to explicitly specify -DCMAKE_<LANG>_CLANG_TIDY from the command line if it is installed somewhere that find_program doesn’t check.

The Clang compiler ID check only guards some code that tries to be helpful and automatically match the compiler version to a corresponding version of clang-tidy (since on at least Debian-based systems multiple Clang versions can be installed side-by-side, with or without an unsuffixed version). find_program should always run to look for a clang-tidy regardless of the compiler.

Having read up on it: removing the license header and replacing it with an SPDX thingy is valid and good. Removing the copyright year, despite other projects doing so too, makes it an invalid copyright notice: it must contain "copyright" or some variation thereof, an author of some sort and the year of first publication. Solution 1: add the year back in. Solution 2: remove the copyright notice altogether, keep only the SPDX line. I'd prefer the latter.

If you want to send me your source on this I will be glad to review it, but I can already assure you that it’s probably not correct (assuming we’ve not time warped back to 1988 :-)). The Berne Convention prohibits any requirement that works be marked for authors to receive copyright protection and mandates a minimum term of copyright of death of the author plus 50 years, or 50 years after publication for anonymous or pseudonymous works, which makes adding years particularly useless. Keeping the shortened licenses in the source code at all was really just to avoid hurting anyone’s feelings (I don’t want anyone to feel like I am trying to erase them or their work); they hold no particularly useful legal weight, especially since separating the license from the code is already a violation.

For the record, I’m not an attorney, but I was once on the board of a 501(c)(6) foundation that acted as a legal administrator for OSS projects. We worked in part with council at IBM; whilst they rejected e-signatures for CLAs until 2012 (12 years after E-SIGN Act), they never raised issues about missing copyright notices in source code. If you want a pretty strong sign that it doesn’t matter, IBM’s lawyers not caring is a pretty good one. :-) The policy of the Linux Foundation is also aligned in this regard. If you are genuinely concerned about any sort of future legal issue, my recommendation is to require CLAs from all current and past contributors, and to find a foundation to act as legal administrator of the project.

Config.cmake.in and helper.cmake are missing license information. And there's currently nothing crediting you in the repo. Time for an AUTHORS file?

My sins are indelibly marked in the VCS and will inevitably come back to haunt me later, and otherwise I really have no ego about it so don’t care if my name shows up somewhere or another. :-) I can add an AUTHORS file if you want, let me know.

The use of directory-level configs made me have nightmarish flashbacks to old CMake versions. After a strong coffee and looking at them being used sensibly, I think this is okay though. Maybe put a small comment on the "disable warnings in this cursed place" property, since it's not too obvious how it works and the places it's used are kinda spread out, connected only by spooky action at a distance.

Boy, if I added a comment to every inscrutable or counter-intuitive thing in CMake that needed a comment, I’m not sure you would be able to see the code any more :-) But I can certainly add something if it feels helpful!

[askmeaboutlo0m]

Well, they compile now. macOS does not support OpenGL ES at all so that part definitely doesn’t work and I am not sure about the rest. :-)

Desktop version runs, anyway! I guess on OSX you'd need ANGLE like on Windows. But not worth looking into, the standalone application is not particularly interesting anymore, considering Drawpile does it way better.

Are you referring to building this repo or the Drawpile repo? This one should require no modifications but please verify in case I need to check against a different Linux distro.

This repo builds fine as-is, just to get it going with Drawpile some small touches were needed.

My apologies for the unclear remarks regarding this change. It is aligned with what you want. Since CMakeLists.txt doesn’t use find_package for these dependencies, it will never try to use a system version. [...]

Ah, in that case, the BUNDLED_* definitions and preprocessor checks are superfluous? There's one for libmypaint and one for parson.

With the emsdk enabled the blessed way is to either run emcmake cmake [...]

Ah yup, that works now, at least as far as compiling and running goes. It doesn't let me type in a username, which is really weird and makes me think something broke on the frontend... which hasn't changed. Ah well, this PR needs small fix due to CMake stupidity, I'll comment the appropriate file.

I know that it is theoretically possible to just use add_subdirectory [...]

Alright, I'd say we'll just leave it as-is for now then, since it works fine, and I'll resolve it when combining the repos. Would presumably make the most sense to combine them into a single cmake project, having it split in two is really just a historical artifact at this point.

If you want to send me your source on this [...]

Apologies, I was unclear: I'm talking about the copyright notice specifically, not the copyright or licensing in general. The copyright situation is fine, the SPDX identifier makes it clear what license that file is under, which is relevant because we got a mixture of GPL and MIT. The copyright notice was just there because it was part of the boilerplate license text. Removing the year makes it improper, so we might as well remove the whole thing instead and leave only the thing that's relevant: a standard way to indicate the license of a file.

Simplest source on what a copyright notice is is probably Wikipedia, but all US and German sources I checked agreed on those three things. Granted, it says should and not must, but I dunno if they RFC-2119-compliant language.

My sins are indelibly marked in the VCS and will inevitably come back to haunt me later, and otherwise I really have no ego about it so don’t care if my name shows up somewhere or another. :-) I can add an AUTHORS file if you want, let me know.

I would like it anyway! The person who contributed Windows and SIMD magic already refused credit and that kinda makes it seem like I did it.

Boy, if I added a comment to every inscrutable or counter-intuitive thing in CMake that needed a comment, I’m not sure you would be able to see the code any more :-) But I can certainly add something if it feels helpful!

It is the one thing in the PR that took me some parsing and cross-referencing anyway! The rest was all thoroughly comprehensible, as far as CMake goes (which can be pretty far, if you throw it hard.)

[csnover]

This repo builds fine as-is, just to get it going with Drawpile some small touches were needed.

OK, yep, cool. That is expected currently.

Ah, in that case, the BUNDLED_* definitions and preprocessor checks are superfluous? There's one for libmypaint and one for parson.

This is just the method I chose to deal with the difference in environment before and after installation. dpengine headers include libmypaint headers, at build time they are at <SOURCE_DIR>/3rdparty/libmypaint, at install time they are at <INSTALL_DIR>/includes/dpengine/libmypaint, and the define facilitates that.

[…] Removing the year makes it improper, so we might as well remove the whole thing instead and leave only the thing that's relevant: a standard way to indicate the license of a file.

Oops, OK. This is what I would normally do with mixed licenses so I am of course good with it. I have made this change.

I would like it anyway! The person who contributed Windows and SIMD magic already refused credit and that kinda makes it seem like I did it.

OK, I pulled authors from the repository history and added it.

Boy, if I added a comment to every inscrutable or counter-intuitive thing in CMake that needed a comment, I’m not sure you would be able to see the code any more :-) But I can certainly add something if it feels helpful!

It is the one thing in the PR that took me some parsing and cross-referencing anyway! The rest was all thoroughly comprehensible, as far as CMake goes (which can be pretty far, if you throw it hard.)

OK, I added comments.

I have also fixed the clang-tidy detection and needlessly prettified the test list output.

[askmeaboutlo0m]

Great! Clang-Tidy works for me out of the box now - a bit too eagerly, will comment in the appropriate place. That and the Emscripten arguments are the only nits remaining, looks all excellent to me and would be ready to merge! Although I'd wait for the mergepocalypse in Drawpile too, just in case it somehow ends up necessitating changes over here too.