cmake: Add dev-mode #1234

real-or-random · 2023-03-10T10:00:14Z

To use, invoke cmake with argument --preset dev-mode.

One disadvantage over ./configure --enable-dev-mode is that CMake does not provide a way to "hide" presets from users. That is, cmake --list-presets will list dev-mode, and it will also appear in cmake-gui, even though it's not selectable there due to a bug in cmake-gui.

Solves one item in #1224.

real-or-random · 2023-03-10T10:12:11Z

CMakePresets.json

+    {
+      // Don't set "generator" here because developers should still be able to
+      // select their preferred generators. As a result we're affected by CMake
+      // bug https://gitlab.kitware.com/cmake/cmake/-/issues/23341, which means
+      // that dev-mode is not selectable in cmake-gui. But in our case, that's
+      // probably rather a feature than a bug.


GitHub highlights these lines because JSON doesn't have comments. But some parsers are very forgiving, and CMake's parser seems to be of that kind.

hebasto · 2023-03-10T10:41:44Z

Concept ACK.

real-or-random · 2023-03-11T14:18:38Z

nb: This is not urgent, but anyway my availability in the next weeks will be restricted. If this requires changes, feel free to take over and make changes in a separate branch / PR.

hebasto · 2023-03-11T16:00:26Z

To use, invoke cmake with argument --preset dev-mode.

Presets are available in CMake 3.19+ only. Should it be documented?

real-or-random · 2023-03-11T17:16:30Z

Presets are available in CMake 3.19+ only. Should it be documented?

I tend to say it's not necessary. It's for devs only and if you invoke cmake --preset on a lower cmake version, I assume it will just error out with CMake Error: Unknown argument as for other unknown arguments.

hebasto

ACK d121958, tested on Ubuntu 22.04.

hebasto · 2023-03-11T19:56:29Z

CMakePresets.json

@@ -0,0 +1,23 @@
+{
+  "version": 3,


That makes this preset usable only with CMake 3.21+.

And CMake 3.19 fails silently when cmake --preset dev-mode.

Hm, I see. Do you have a suggestion? I don't think we can do much about it, except hope that noone tries to use this with 3.19 or 3.20.

As it is a developer-specific feature, I'm OK with the current patch.

hebasto · 2023-03-13T15:29:50Z

Maybe add /CMakeUserPresets.json to the .gitignore?

hebasto · 2023-03-13T15:54:07Z

Well, cmake --preset dev-mode will place the build artifacts in the source tree, which is discouraged.

There are two options to avoid such behavior:

cmake --preset dev-mode -B build or
add "binaryDir": "${sourceDir}/build" to the CMakePresets.json

real-or-random · 2023-03-16T14:01:20Z

Maybe add /CMakeUserPresets.json to the .gitignore?

Done. I took the version without the leading slash because this file could in principle exist in src/ or examples/ too.

Well, cmake --preset dev-mode will place the build artifacts in the source tree, which is discouraged.

Okay, yes, this command was just intended as an example.

There are two options to avoid such behavior:

cmake --preset dev-mode -B build or

add "binaryDir": "${sourceDir}/build" to the CMakePresets.json

My thinking is that, while ${sourceDir}/build is typically used, this should remain a local decision of the user/developer and not be the responsibility of the template. For example, if someone wants to maintain multiple for testing etc. ... I guess the binaryDir is rather useful for automated builds, e.g, to facilitate CI configs.

hebasto

ACK e8a4a94

hebasto · 2023-03-16T14:13:17Z

.gitignore

 libsecp256k1.pc

+### CMake
+CMakeUserPresets.json


Maybe add /CMakeUserPresets.json to the .gitignore?

Done. I took the version without the leading slash because this file could in principle exist in src/ or examples/ too.

https://cmake.org/cmake/help/latest/manual/cmake-presets.7.html:

CMakePresets.json and CMakeUserPresets.json live in the project's root directory.

Ok, you're right, fixed.

hebasto · 2023-03-16T14:15:25Z

My thinking is that, while ${sourceDir}/build is typically used, this should remain a local decision of the user/developer and not be the responsibility of the template.

Command line options allow the user/developer to override a template.

hebasto

re-ACK 712090f

real-or-random · 2023-03-16T14:32:51Z

My thinking is that, while ${sourceDir}/build is typically used, this should remain a local decision of the user/developer and not be the responsibility of the template.

Command line options allow the user/developer to override a template.

Would you prefer this to be added? I'm fine with either, but I still tend to think that this should not be the concern of the template. Pragmatically speaking, it doesn't make a big difference, of course.

hebasto · 2023-03-16T14:41:12Z

My thinking is that, while ${sourceDir}/build is typically used, this should remain a local decision of the user/developer and not be the responsibility of the template.

Command line options allow the user/developer to override a template.

Would you prefer this to be added? I'm fine with either, but I still tend to think that this should not be the concern of the template. Pragmatically speaking, it doesn't make a big difference, of course.

Let's merge it in its current state :)

real-or-random · 2023-04-18T11:08:50Z

@theuni Would you like to review this? :)

theuni · 2023-04-18T15:32:32Z

Presets are available in CMake 3.19+ only. Should it be documented?

I tend to say it's not necessary. It's for devs only and if you invoke cmake --preset on a lower cmake version, I assume it will just error out with CMake Error: Unknown argument as for other unknown arguments.

I just tested with 3.16.3, and it just silently swallows the unknown option and continues. Since I would otherwise have assumed it worked, I agree it's worth adding a quick note about required version.

theuni

ACK modulo comment nit.

real-or-random · 2023-04-19T10:41:21Z

Updated to add a comment on the required CMake version.

hebasto · 2023-04-20T15:53:05Z

CMakePresets.json

+  // Note: JSON does not support officially, but CMake apparently
+  // ignores lines starting with whitespace followed by "//" silently.


Digging into the CMake repository brings some discussions about that:

https://gitlab.kitware.com/cmake/cmake/-/issues/21858

https://gitlab.kitware.com/cmake/cmake/-/merge_requests/5853

Urghs, thanks for doing the research here. So they first enabled comment support intentionally, then disabled it intentionally. Currently, it just works by accident.

I'm not sure how we should deal with this. Having the comments is risky because future versions may not support them. (This PR here uses presets only for dev mode, but we may add other presets, e.g., for coverage....)

It seems that CMake's parser defeats all the hacks I could imagine or find in https://stackoverflow.com/questions/244777/can-comments-be-used-in-json. Things we could do:

Have a template file and strip // lines using grep, generating the actual .json from it. But this needs some mechanism to make sure that the file is up to date etc... All doable but seems overkill for this tiny issue.

(https://hjson.github.io/ is an entire off-the-shelf solution for this, but that adds a maintainer dependency and is hard to remember.)

We could just drop the comments. CMake templates support a cmakeMinimumRequired field which I think is enough to document the need for CMake 3.21. A better warning should then be added to build docs (to be written :D). The comment about the missing generator thing can be in the commit message (not optimal but okayish).

I lean towards dropping comments. Sad that we need to spend time on this...

I lean towards dropping comments.

Agree.

To use, invoke `cmake` with argument `--preset dev-mode`. Solves one item in bitcoin-core#1235. One disadvantage over `./configure --enable-dev-mode` is that CMake does not provide a way to "hide" presets from users. That is, `cmake --list-presets` will list dev-mode, and it will also appear in `cmake-gui`, even though it's not selectable there due to bug https://gitlab.kitware.com/cmake/cmake/-/issues/23341. (So in our case, that's probably rather a feature than a bug.) We curently use version 3 presets which require CMake 3.21+. Unfortunately, CMake versions before 3.19 may ignore the `--preset` argument silently. So if the preset is not picked up, make sure you have a recent enough CMake version. More unfortunately, we can't even spell this warning out in CMakePresets.json because CMake does not support officially support comments in JSON, see - https://gitlab.kitware.com/cmake/cmake/-/issues/21858 - https://gitlab.kitware.com/cmake/cmake/-/merge_requests/5853 . We could use a hack hinted at in https://gitlab.kitware.com/cmake/cmake/-/issues/21858#note_908543 but that's risky, because it could simply break for future versions, and we probably want to use presets not only for dev mode.

This file is specifically intended for *local* CMake templates (as opposed to CMakePresets.json).

real-or-random · 2023-04-20T17:25:09Z

Forced-push to remove the comments and move their contents to the commit message.

hebasto

ACK ce5ba9e

hebasto · 2023-04-21T16:38:01Z

CMakePresets.json

@@ -0,0 +1,19 @@
+{
+  "cmakeMinimumRequired": {"major": 3, "minor": 21, "patch": 0}, 
+  "version": 3,


A note for reviewers: version 3 is required as we omit the generator entry.

theuni

ACK ce5ba9e

real-or-random commented Mar 10, 2023

View reviewed changes

hebasto mentioned this pull request Mar 10, 2023

build: Meta-issue for follow-ups to initial CMake merge (#1113) #1235

Closed

14 tasks

hebasto approved these changes Mar 11, 2023

View reviewed changes

hebasto approved these changes Mar 16, 2023

View reviewed changes

real-or-random force-pushed the 202303-cmake-dev-mode branch from e8a4a94 to 712090f Compare March 16, 2023 14:30

hebasto approved these changes Mar 16, 2023

View reviewed changes

hebasto mentioned this pull request Mar 28, 2023

cmake: Use "coverage" preset instead of "Coverage" build type and document it #1251

Closed

theuni approved these changes Apr 18, 2023

View reviewed changes

real-or-random force-pushed the 202303-cmake-dev-mode branch 2 times, most recently from 47bab0f to 50ef165 Compare April 19, 2023 10:41

hebasto reviewed Apr 20, 2023

View reviewed changes

real-or-random added 2 commits April 20, 2023 19:22

gitignore: Add CMakeUserPresets.json

ce5ba9e

This file is specifically intended for *local* CMake templates (as opposed to CMakePresets.json).

real-or-random force-pushed the 202303-cmake-dev-mode branch from 50ef165 to ce5ba9e Compare April 20, 2023 17:23

hebasto approved these changes Apr 21, 2023

View reviewed changes

real-or-random requested a review from theuni April 25, 2023 15:22

theuni approved these changes Apr 27, 2023

View reviewed changes

real-or-random merged commit 596b336 into bitcoin-core:master Apr 27, 2023

		// Note: JSON does not support officially, but CMake apparently
		// ignores lines starting with whitespace followed by "//" silently.

Uh oh!

cmake: Add dev-mode #1234

cmake: Add dev-mode #1234

Uh oh!

Conversation

real-or-random commented Mar 10, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Mar 10, 2023

Uh oh!

real-or-random commented Mar 11, 2023

Uh oh!

hebasto commented Mar 11, 2023

Uh oh!

real-or-random commented Mar 11, 2023

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto Mar 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Mar 13, 2023

Uh oh!

hebasto commented Mar 13, 2023

Uh oh!

real-or-random commented Mar 16, 2023

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Mar 16, 2023

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

real-or-random commented Mar 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Mar 16, 2023

Uh oh!

real-or-random commented Apr 18, 2023

Uh oh!

theuni commented Apr 18, 2023

Uh oh!

theuni left a comment

Choose a reason for hiding this comment

Uh oh!

real-or-random commented Apr 19, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

real-or-random commented Apr 20, 2023

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theuni left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

hebasto Mar 11, 2023 •

edited

Loading

real-or-random commented Mar 16, 2023 •

edited

Loading