Redesign CMake syntax
ClosedPublic

Authored by turbov on Aug 7 2017, 6:20 AM.

Details

Summary

CMake syntax has been redesigned to pursue the following goals:

  • the former generator has used an output of cmake --help (and friends) commands, which is quite informal (not a well-formed) and error prune
  • the current syntax was really out of date

The new approach is to use a trivial Python script, which uses a prepared YAML data file and Jinja template to render the resulting syntax file.
Key features of the new implementation are:

  • every command highlight only related named parameters -- e.g. add_library do not have TARGET named option, so it doesn't highlight in this context
  • new highlight classes have introduced two distinct property names, aliased (imported) targets, special non-named arguments...
  • reuse RST syntax for comments
  • input data now in VCS and trackable

there is still some work to do, but I've just wanted to know is there any interest in this approach...

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
turbov created this revision.Aug 7 2017, 6:20 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptAug 7 2017, 6:20 AM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
cullmann added a subscriber: cullmann.

If that makes it easier to keep the syntax file up-to-date, I would have no issues with a generator build in this way.
If we ever want to go away from it, we still have the generated file around as base.
But Volker should give his input, too.

dhaumann edited edge metadata.EditedAug 7 2017, 8:01 AM

I see seveal issues that imo should be addressed:

  1. Please add a test case in autotest/input/ or extend it if one already exists (did you run make test?)
  1. How does it work, are we supposed to run this command from time to time manually? If so, then I'd be ok with that.
  1. How high is the maintenance burden over time: I understand that this should this, but given we have a yaml file that needs to be maintained manually, I wonder whether we simply move the maintenance from one place to another and additionally introduce complexity (in terms of additional tooling) one first needs to understand before being able to fix things.
  1. The kateversion is back to 2.4 and hard-coded colors are used again. Previously, it was set to 5.0, and used already the newly introduced default styles. I strongly suggest to keep the new default styles - we purposefully changed this some time ago, and I dislike that fact that we go a step back here.

I see seveal issues that imo should be addressed:

  1. Please add a test case in autotest/input/ or extend it if one already exists (did you run make test?)

no, I don't run tests yet (it is too complicated to build the repo for me nowadays)

  1. How does it work, are we supposed to run this command from time to time manually? If so, then I'd be ok with that.

it work pretty simple:

  1. reading YAML (as a Python's dict)
  2. prepare (rename keys, & etc) data for renderer
  3. render data (a dict) into the Jinja template

as I said the previous generator became unstable -- parsing the output of cmake --help is not reliable, so this generator uses "static" YAML file, which should be updated manually

  1. How high is the maintenance burden over time: I understand that this should this, but given we have a yaml file that needs to be maintained manually, I wonder whether we simply move the maintenance from one place to another and additionally introduce complexity (in terms of additional tooling) one first needs to understand before being able to fix things.

the point w/ the current generator: I'm far from sure it "parse" everything correct... moreover, I know exactly that it "parses" cmake --help output incorrectly (and one have to fix the generated syntax manually after the generator) and there is really hard to make the generator right cuz the informal/irregular nature of help screens.
in contrast, the new approach offers a full control over the generated syntax, yeah, by the cost of "manual" maintenance of the YAML file (I tried to make it as simple as possible, and I'm sure it shouldn't be a problem to anyone to add anything to it %)
Usually, I read CMake's ChangeLog carefully after any new release, spotting new commands/variables/properties/etc and this is the time to update syntax (YAML file)...

  1. The kateversion is back to 2.4 and hard-coded colors are used again. Previously, it was set to 5.0, and used already the newly introduced default styles. I strongly suggest to keep the new default styles - we purposefully changed this some time ago, and I dislike that fact that we go a step back here.

previous cmake.xml do not use anything from modern kate, this syntax works fine w/ KDE4 version of kate.
anyway, it would be easy to fix it and remove hardcoded colors anyway...

the hardest part is to realize would this approach be accepted or not

turbov updated this revision to Diff 18322.Aug 18 2017, 6:48 AM

Remove spaces around <item>

dhaumann added a comment.EditedAug 22 2017, 8:00 PM

What is still unclear to me: As I understand we from time to time run the generator script (point 2 in my list). So this is not automatically run when executing "make", right?

I would be fine with this change, but the other point still stand: remove hard-coded colors :-) And do we have a test case for this? If not, please add a test file.
Also, since you redesign this file, I think it would be OK to relicense to MIT.

@vkrause What's your take on this?

What is still unclear to me: As I understand we from time to time run the generator script (point 2 in my list). So this is not automatically run when executing "make", right?

correct. it doesn't run on make and never was. Some time ago I've imroved that generator and know its limitations and pitfals -- yeah, it is too simple to "parse" informal (in fact) output of cmake --help-xxx commands correctly.
That is the main goal I pursue doing that redesign -- to make sure that all commands and parameters are here (in keyword lists) is not a mistake (error in the current generator).
And most important, at least to me, to highlight only really existed named keywords for particular command, giving a hint to a user that particular parameter is really exists in this particular command.

I would be fine with this change, but the other point still stand: remove hard-coded colors :-) And do we have a test case for this? If not, please add a test file.

yeah, I'll do that as soon as realize that this MR have a chance to be accepted... %)

Also, since you redesign this file, I think it would be OK to relicense to MIT.

sure

turbov updated this revision to Diff 21207.Oct 23 2017, 11:59 PM
turbov updated this revision to Diff 21208.Oct 24 2017, 12:06 AM

Include updates for CMake 3.10

turbov updated this revision to Diff 22226.Nov 12 2017, 12:39 PM
turbov edited the summary of this revision. (Show Details)

Updated for CMake 3.10

turbov updated this revision to Diff 23100.Nov 28 2017, 8:31 PM
turbov added a comment.Feb 5 2018, 5:40 AM

@cullmann , @dhaumann

if there is no intereset, I'll close this review and will distribute my CMake syntax in other ways...

If the new highlighting better covers what CMake has around on features now than the old, I am for adding the new one.

If the generator is optimal or not is an other thing, but as long as we would just check in the result and have the generator as addition to be able to rerun on demand, I think that is no issue.
(if the file is handcraftet updates are much harder)

But Volker may decide as maintainer of the framework.

I'm with Christoph on this. Having the generated code checked in is fine, I'd like to avoid running this on compile time due to the extra dependencies. While that's not a big deal on a Linux desktop machine, things are different when cross-compiling or when on Windows.

turbov added a comment.Feb 7 2018, 9:37 AM

I'm with Christoph on this. Having the generated code checked in is fine, I'd like to avoid running this on compile time due to the extra dependencies.

It doesn' needed to run anything @ compile time.

Just to clarify things once again:

  • The generator use only YAML as input
  • The YAML file is manually maintained! (cuz grabbing output of cmake --help... doesn't work well for a long time already)
  • I do updates of YAML w/ every CMake release after reading release notes line by line (cuz I do a lot of CMake code in may daily job and use the latest version all the time)

If that is OK 4 u guys, I'll commit...

turbov updated this revision to Diff 26933.Feb 11 2018, 2:39 PM

Updates for CMake 3.11

dhaumann accepted this revision.Mar 10 2018, 8:20 PM

So everyone seems to agree that in general it's a good idea.

@turbov Please go ahead and commit. Don't forget to run make test, and possibly update the unit test rererence with autotests/update-reference-data.sh. Maybe you even want to extend the autotest/input/*.cmake files. Please maintain it also for the future :-)

This revision is now accepted and ready to land.Mar 10 2018, 8:20 PM

Oh, and btw. sorry that this review took essentially 7 months. Sometimes we simply suck and things go badly wrong :-)

cullmann accepted this revision.Apr 15 2018, 11:13 AM

Yes, please push your change.
Thanks for the work!

cullmann closed this revision.Apr 15 2018, 12:31 PM

This is already pushed ;=)

Autor: Alex Turbov <i.zaufi@gmail.com> 2017-08-07 08:01:46
Eintragender: Alex Turbov <i.zaufi@gmail.com> 2018-03-28 21:17:34
Eltern: aa5a8e08e0a2004536ab87531efbbbb93217cf4a (GIT_SILENT Upgrade Qt5 version requirement to 5.8.0.)
Kind: 683fc509a1ad5ac36e2446cf35a123f479455636 (Make it possible to fully build the project when crosscompiling)
Zweig: master, remotes/origin/master
Folgt auf: v5.30.0-rc1
Vorgänger von: v5.45.0, v5.45.0-rc1

Redesign CMake syntax generator

The new approach is to use a trivial Python script, which uses a prepared YAML
data file and Jinja template to render the resulting syntax file.
Key features of the new implementation are:

- every command highlights only related named parameters -- e.g. `add_library`
  do not have `TARGET` named option, so it doesn't highlight in this context;
- new highlight classes have introduced to distinct property names, aliased
  (imported) targets, special non-named arguments;
- reuse RST syntax for comments;
- the input data now in VCS and trackable.

Sure? It was reverted a month ago once. Was it applied again?