Closed dcbaker closed 4 years ago
I'm considering this RFC at the moment because I'd like some feedback on the API portion, namely, does it make more sense to use the special case dict structure, or to allow summary()
to take arbitrary kwargs for sections and always print dictionaries? I'm kinda leaning toward the latter now that I look at it more.
I switched to the keyword argument variant, which always treats dicts as dicts with no special meaning based on their level. I think I like this better as it's more consistent.
I think from an API point of view I prefer the "take arbitrary kwargs for sections and always print dictionaries" variant to the dict-inside-dicts variant. Just looks nicer and cleaner and also discourages nesting of sections.
We're in agreement then, and should ask @jpakkane how he feels about the api?
Interestingly I just manage to beat you by one day: #4643
It's a lot simpler, though...
Which one is simpler?
The one thing the other MR does that this one does not is to store the summary per subproject and print all of them in one go at the very end (with the master project last and subprojects in alphabetical order).
That's how this one is supposed to work (minus the alphabetical order), but I change that.
Apparently there are bugs :)
Updated to sort subprojects alphabetically, print the main project last, and not print subproject summaries during subproject initialization.
The thing I don't like in this PR is the free-form keyword arguments. That means we cannot extend this function with other options. For example we'll never be able to add summary(..., colored : true) because that would print "colored=true" instead of being a setting of the summary function. I would rather have nested dict (with max depth to 2) to be future-proof.
From a readability POV, it makes no difference, it looks even more consistent to me. Allow multiple positional arguments that must be all dictionaries:
summary(
{'Backend' : 'OpenGL'},
{'Server' : sec1},
{'Client' : sec2},
{'Misc' : sec3},
)
Also that allows the incrementally construct the summary:
summary = [{'Backend' : 'OpenGL'}]
if something
summary += {'foo' : 'bar'}
endif
summary(summary)
So it would be the same to have 1 list positional argument, or N dict positional arguments. Many functions works that way already.
I tried that originally, and I don't like it because it makes dicts mean 2 things, for two levels of nesting they mean "indent this" but after that they don't:
foo = {'one' : {'two' : {'three' : {'four': 'foo'}}}}
summary(foo)
would print as:
one:
two:
three: {'four': 'foo'}
but
foo = {'one' : {'two' : {'three' : 'foo'}}}
prints as:
one:
two:
three: 'foo'
Which I feel violates least-surprise.
I agree that maintaining keywords for API use would be better, so how about:
summary({'Backend' : 'OpenGL'}, sections : {'Server' : sec1, 'Client' : sec2, 'Misc': sec3})
Which remove the free-form keywords, but keeps the behavior of nested dictionaries consistent.
summary({'Backend' : 'OpenGL'}, sections : {'Server' : sec1, 'Client' : sec2, 'Misc': sec3})
That's nested dictionaries too, I think the advantage of my proposal is that sections are in a list, so user controls the ordering.
foo = {'one' : {'two' : {'three' : {'four': 'foo'}}}}
foo = {'one' : {'two' : {'three' : 'foo'}}}
Of course put like that it's confusing, but with intermediary variables and a bit of indentation, it's not that bad. Taking your initial example:
sec1 = {'driver' : 'foobar', 'OS' : 'Linux', 'API' : 1.7}
sec2 = {'driver' : 'dive comp', 'OS' : 'Minix', 'API' : 1.1.2}
sec3 = {'with' : {'mesa' : true, 'gbm' : false}}
summary = [
{'Backend' : 'OpenGL'},
{'Server' : sec1},
{'Client' : sec2},
{'Misc' : sec3},
]
One thing I would like to do with keyword arguments, is enabling some automatic sections (doesn't need to be done in initial PR). One example that comes to mind is listing all subprojects with a YES/NO value to tell if they worked.
summary(subprojects: true)
Would print:
Subprojects:
gst-plugins-good: Yes
gst-plugins-bad: Failed
gst-plugins-ugly: Disabled
Another extention can that be done later, is passing dependency objects
libfoo = dependency('foo', required : get_option('foo_feature'))
summary({'Dependencies' : [libfoo, libbar]})
Would print:
Dependencies:
foo: Yes
bar: Not found
baz: Not found (feature 'baz' disabled)
All of this doesn't need to be implemented immediately, but I would like to make sure we keep in mind that this will likely be extended in the future, so we have to design it flexible.
Do meson's dicts have an ordering guarantee? Is that behavior just leaking from python 3.7, or do we mean to have it?
There is no ordering guarantee.
I guess that leaves us with either @xclaesse's list of lists (which I'm not a huge fan of, but I could live with I think), making dictionaries ordered (Because python guarantees it now, people will start relying on it so it might be worth us doing), or going to a more complicated object, maybe a module? Does anyone have preferences?
(Just thinking out loud here...)
Maybe we should take a step back and revisit some of the original ideas floating about in e.g. #757. I think we're perhaps trying to do too much at once here, trying to squeeze everything into a single summary()
function.
How about something where a single summary()
would handle the simple case of one single section (or no sections at all rather), but it would also return an object where one could then add sections with a name and a dict as one of the arguments? That's somewhere in between the original proposals where every single line item was a method, which perhaps was a bit tedious/verbose, and the everything-in-a-dict approach?
Or even simpler: let there be only a single summary()
function with one call to summary()
for each section and two positional parameters (title and a dict)?
Or even simpler: let there be only a single
summary()
function with one call tosummary()
for each section and two positional parameters (title and a dict)?
We already have to support multiple summary()
calls (one per subproject) so that makes sense. I would rather have the title in an optional kwarg instead of positional arg because some key/value are not under a section.
I personally still prefer my proposal from https://github.com/mesonbuild/meson/pull/4649#issuecomment-449163275, but if people prefer @tp-m's proposal, I've got no strong opinion against it.
@tp-m, could you give me a psudeo-code example of what you're thinking?
@tp-m, could you give me a psudeo-code example of what you're thinking?
I guess something like that:
sec1 = {'driver' : 'foobar', 'OS' : 'Linux', 'API' : 1.7}
sec2 = {'driver' : 'dive comp', 'OS' : 'Minix', 'API' : 1.1.2}
sec3 = {'with' : {'mesa' : true, 'gbm' : false}}
summary({'Backend' : 'OpenGL'})
summary(sec1, title : 'Server')
summary(sec2, title : 'Client')
summary(sec3, title : 'Misc')
@tp-m, could you give me a psudeo-code example of what you're thinking?
Something like this I think:
Single section:
summary('gdk-pixbuf', { 'loaders' : ['jpeg', 'png', 'tiff'], 'MMX' : true, 'Altivec' : false })
Multiple sections:
summary('Server', {'driver' : 'foobar', 'OS' : 'Linux', 'API' : 1.7})
summary('Client', {'driver' : 'dive comp', 'OS' : 'Minix', 'API' : 1.1.2})
summary('Misc', {'with' : {'mesa' : true, 'gbm' : false}})
(not sure what the behaviour of the nesting should be here in the Misc section, but that's details ;))
Okay, I'll update this today and see how the multiple calls to summary works out.
Since this is a new function, please also add this
'summary': self.func_do_nothing,
to the self.funcs.update
call in the AstInterpreter
(somwhere around here) before this gets merged.
I can do that, thanks for letting me know about that.
I've updated with the AstInterpreter
changes and @xclaesse's list of list approach.
I prefer the list of list to the dicts because lists have guaranteed order unlike dicts.
@dcbaker I rebased your commit, and added an extra commit with fixes for my review comments above. Let me know if that's what you had in mind.
I can't review my own PR, but with the sider warning fixed (I thinks it's a missing r
prefix), you can have my r-b.
Probably should get @jpakkane to sign off though.
I can't review my own PR, but with the sider warning fixed (I thinks it's a missing
r
prefix), you can have my r-b.
Already fixed, it was that indeed.
It would probably look better if the values were indented into columns. Something like:
Main Project:
Backend OpenGL
Server
driver foobar
OS Linux
API 1.7
Client
driver dive comp
OS Minix
API 1.1.2
@jpakkane we can always tweak the look, but if you agree with the API we could already merge this?
Looks basically fine by me, but the big question is does it provide all (or at least most) of the things people need it for. For example systemd has a fairly extensive status printing thingy, maybe someone like @keszybz could comment if this proposed approach would work for them. I'm guessing GStreamer and Mesa requirements are already taken care of.
@jpakkane I would be happy to hear from them indeed. otoh, I don't think we can expect to catch all use-cases on the first try. We'll surely extend this over time. That's why I wanted to be sure we can still use summary() kwargs to add options, etc.
For the record, systemd prints it like that:
Message: systemd 244
split /usr: true
split bin-sbin: true
prefix directory: /opt/gnome
rootprefix directory: /
sysconf directory: /etc
include directory: /opt/gnome/include
lib directory: /opt/gnome/lib/x86_64-linux-gnu
rootlib directory: /x86_64-linux-gnu
SysV init scripts: /etc/init.d
SysV rc?.d directories: /etc/rc.d
PAM modules directory: /x86_64-linux-gnu/security
PAM configuration directory: /etc/pam.d
RPM macros directory: /opt/gnome/lib/rpm/macros.d
modprobe.d directory: /lib/modprobe.d
D-Bus policy directory: /opt/gnome/share/dbus-1/system.d
D-Bus session directory: /opt/gnome/share/dbus-1/services
D-Bus system directory: /opt/gnome/share/dbus-1/system-services
bash completions directory: /usr/share/bash-completion/completions
zsh completions directory: /opt/gnome/share/zsh/site-functions
extra start script: /etc/rc.local
debug shell: /bin/sh @ /dev/tty9
TTY GID: 5
users GID: -
maximum system UID: 999
maximum system GID: 999
minimum dynamic UID: 61184
maximum dynamic UID: 65519
minimum container UID base: 524288
maximum container UID base: 1878982656
/dev/kvm access mode: 0666
render group access mode: 0666
certificate root directory: /etc/ssl
support URL: https://lists.freedesktop.org/mailman/listinfo/systemd-devel
nobody user name: nobody
nobody group name: nobody
fallback hostname: localhost
symbolic gateway hostnames: _gateway
default DNSSEC mode: allow-downgrade
default DNS-over-TLS mode: no
default cgroup hierarchy: unified
default net.naming-scheme setting: latest
default KillUserProcesses setting: true
default locale: C.UTF-8
default user $PATH: (same as system services)
systemd service watchdog: 3min
default DNS servers: 1.1.1.1
8.8.8.8
1.0.0.1
8.8.4.4
2606:4700:4700::1111
2001:4860:4860::8888
2606:4700:4700::1001
2001:4860:4860::8844
default NTP servers: time1.google.com
time2.google.com
time3.google.com
time4.google.com
time epoch: 1576009432 (2019-12-10T20:23:52+00:00)
static libsystemd: false
static libudev: false
efi arch: x86_64
enabled features: IMA, SELinux, SMACK, zlib, xz, bzip2, gcrypt, gnutls, libcurl, idn, libidn2, binfmt, vconsole, quotacheck, tmpfiles, environment.d, sysusers, firstboot, randomseed, backlight, rfkill, logind, machined, portabled, importd, hostnamed, timedated, timesyncd, localed, networkd, resolve, coredump, pstore, polkit, efi, xkbcommon, blkid, dbus, glib, nss-myhostname, nss-mymachines, nss-resolve, nss-systemd, hwdb, tpm, SysV compat, utmp, ldconfig, hibernate, adm group, wheel group, gshadow, link-udev-shared, link-systemctl-shared
disabled features: libcryptsetup, PAM, AUDIT, AppArmor, SECCOMP, lz4, ACL, qrencode, microhttpd, openssl, libidn, libiptc, elfutils, DNS-over-TLS(gnutls), DNS-over-TLS(openssl), legacy pkla, gnu-efi, kmod, pcre2, man pages, html pages, man page indices, debug hashmap, debug mmap cache, debug siphash, valgrind, trace logging
Another interesting, gtk-doc:
gtk-doc 1.32.1
Directories:
prefix: /opt/gnome
bindir: /opt/gnome/bin
libdir: /opt/gnome/lib/x86_64-linux-gnu
datadir: /opt/gnome/share
Configuration:
Autotools support: true
CMake support: true
PDF output: true
User manual: true
Test suite: true
I don't think we necessary need to support each output format, even on the contrary it's better to uniform. IMHO, the only question is can we output the same info.
From a personal taste, I don't really like our current unaligned key = value
. I don't like systemd's column neither because key being far from value it's hard to follow the line. I actually like gtk-doc's center alignment.
meson.build:3161:0: ERROR: Options for section 'status' already set.
I think it'd be more user-friendly to merge instead, if the same section name is specified twice, and only warn or error out if the same dict key appears twice in the same section. For example when subdirectories are used, it is much more convenient to add items to the status in a few different places then to manually collect everything in a dictionary and only call summary() at the end. And the same goes for conditional output, e.g. architecture dependent.
I tried using this with systemd, and the output looks like this:
Main Project:
status
systemd = '244'
split /usr = False
split bin-sbin = True
prefix directory = '/usr'
rootprefix directory = '/usr'
sysconf directory = '/etc'
include directory = '/usr/include'
lib directory = '/usr/lib64'
rootlib directory = '/usr/lib64'
SysV init scripts = '/etc/init.d'
SysV rc?.d directories = '/etc/rc.d'
PAM modules directory = '/usr/lib64/security'
PAM configuration directory = '/etc/pam.d'
RPM macros directory = '/usr/lib/rpm/macros.d'
modprobe.d directory = '/usr/lib/modprobe.d'
D-Bus policy directory = '/usr/share/dbus-1/system.d'
D-Bus session directory = '/usr/share/dbus-1/services'
D-Bus system directory = '/usr/share/dbus-1/system-services'
bash completions directory = '/usr/share/bash-completion/completions'
zsh completions directory = '/usr/share/zsh/site-functions'
extra start script = '/etc/rc.local'
debug shell = '/bin/sh @ /dev/tty9'
TTY GID = 5
users GID = '-'
maximum system UID = 999
maximum system GID = 999
minimum dynamic UID = 61184
maximum dynamic UID = 65519
minimum container UID base = 524288
maximum container UID base = 1878982656
/dev/kvm access mode = '0666'
render group access mode = '0666'
certificate root directory = '/etc/ssl'
support URL = 'https://lists.freedesktop.org/mailman/listinfo/systemd-devel'
nobody user name = 'nobody'
nobody group name = 'nobody'
fallback hostname = 'localhost'
symbolic gateway hostnames = '_gateway'
default DNSSEC mode = 'allow-downgrade'
default DNS-over-TLS mode = 'no'
default cgroup hierarchy = 'unified'
default net.naming-scheme setting = 'latest'
default KillUserProcesses setting = True
default locale = 'C.UTF-8'
default user $PATH = '(same as system services)'
systemd service watchdog = '3min'
default DNS servers = '1.1.1.1\n 8.8.8.8\n 1.0.0.1\n 8.8.4.4\n 2606:4700:4700::1111\n 2001:4860:4860::8888\n 2606:4700:4700::1001\n 2001:4860:4860::8844'
default NTP servers = 'time1.google.com\n time2.google.com\n time3.google.com\n time4.google.com'
time epoch = '1576062304 (2019-12-11T11:05:04+00:00)'
static libsystemd = 'false'
static libudev = 'false'
EFI
efi arch = 'x86_64'
EFI machine type = 'ccache cc'
EFI lib directory = '/usr/lib64'
EFI lds directory = '/usr/lib64/gnuefi'
EFI include directory = '/usr/include/efi'
enablement status
enabled features = 'libcryptsetup, PAM, AUDIT, IMA, SELinux, SECCOMP, SMACK, zlib, xz, lz4, bzip2, ACL, gcrypt, qrencode, microhttpd, gnutls, openssl, libcurl, idn, libidn2, libiptc, elfutils, binfmt, vconsole, quotacheck, tmpfiles, environment.d, sysusers, firstboot, randomseed, backlight, rfkill, logind, machined, portabled, importd, hostnamed, timedated, timesyncd, localed, networkd, resolve, DNS-over-TLS(gnutls), coredump, pstore, polkit, efi, gnu-efi, kmod, xkbcommon, pcre2, blkid, dbus, glib, nss-myhostname, nss-mymachines, nss-resolve, nss-systemd, hwdb, tpm, SysV compat, utmp, ldconfig, hibernate, adm group, wheel group, gshadow, valgrind, link-udev-shared, link-systemctl-shared'
disabled features = 'AppArmor, libidn, DNS-over-TLS(openssl), legacy pkla, man pages, html pages, man page indices, debug hashmap, debug mmap cache, debug siphash, trace logging'
It is not bad. It is definitely much less work than the current hand-crafted summary we have. Nevertheless, it is not as readable as the hand-crafted yet, and I wouldn't want to switch from the handcrafted status to this until some things are improved.
Main Project
I'd prefer this to be something like "\<project-name> \<project-version> summary" instead. For projects that don't use subprojects this line is noise. Or maybe make this configurable though a keyword argument.
For the dictionary, I think just using the default dict formatter is not going to be good enough.
systemd = '244'
split /usr = False
split bin-sbin = True
prefix directory = '/usr'
It would be better to print the strings without quotes. Many things are internally strings but are never displayed quoted, like version numbers. Having the quotes on some settings or not others is visually displeasing. The same goes for stuff like "(not set)" or similar — they must be displayed without quotes.
default DNS servers = '1.1.1.1\n 8.8.8.8\n 1.0.0.1\n 8.8.4.4\n 2606:4700:4700::1111\n 2001:4860:4860::8888\n 2606:4700:4700::1001\n
For strings with quotes, it would be great if they could be split at newlines and aligned to the "=" sign. I.e display the first version as:
default DNS servers = 1.1.1.1
8.8.8.8
...
2606:4700:4700::1001
I also tried a version with a list:
default DNS servers = ['1.1.1.1', '8.8.8.8', '1.0.0.1', '8.8.4.4', '2606:4700:4700::1111', '2001:4860:4860::8888', '2606:4700:4700::1001', '2001:4860:4860::8844']
This looks better out of the box, but is not very readable. Breaking the list up and aligning to "=" would be great.
Maybe use color for the section headers?
I pushed my test as https://github.com/keszybz/systemd/pull/new/meson-summary in case anyone wants to play with it.
meson.build:3161:0: ERROR: Options for section 'status' already set.
I think it'd be more user-friendly to merge instead, if the same section name is specified twice, and only warn or error out if the same dict key appears twice in the same section.
Agreed, I'll change that.
Main Project
I'd prefer this to be something like "
summary" instead. For projects that don't use subprojects this line is noise. Or maybe make this configurable though a keyword argument.
Agreed. If we go with <project-name> <project-version>
instead of Main Project
the it's not noise any more even when there are no subprojects, since that tells the version which is useful information. I think I should also add the version for subprojects while at it.
For the dictionary, I think just using the default dict formatter is not going to be good enough.
systemd = '244'
split /usr = False
split bin-sbin = True
prefix directory = '/usr'
It would be better to print the strings without quotes. Many things are internally strings but are never displayed quoted, like version numbers. Having the quotes on some settings or not others is visually displeasing. The same goes for stuff like "(not set)" or similar — they must be displayed without quotes.
Agreed. When the value is a simple string the quoting is just noise. But when the value is array/dict then it could be needed maybe.
For strings with quotes, it would be great if they could be split at newlines and aligned to the "=" sign.
I don't think we should try to parse strings, the point here is project would have to use an array of string instead, as you did below.
default DNS servers = ['1.1.1.1', '8.8.8.8', '1.0.0.1', '8.8.4.4', '2606:4700:4700::1111', '2001:4860:4860::8888', '2606:4700:4700::1001', '2001:4860:4860::8844']
This looks better out of the box, but is not very readable. Breaking the list up and aligning to "=" would be great.
Agreed, I like how systemd breaks that list into lines, that could be the default behaviour when value is a list.
Maybe use color for the section headers?
Agreed, but I'm not sure which color :) Speaking about colors, I also wanted a mode where booleans would turn into YES/NO in green/red, but that can be future improvement.
@keszybz I updated the PR with those suggestions. I dropped 2 use-cases to simplify the code:
I'm considering adding summary(dict)
syntax (dict without section) for the case you want to skip the section name, as systemd does. And maybe summary(section, key, value)
in the case you want to add key/value one by one. And while at it, summary(key, value)
in the case you want to add key/value one by one without a section. But those syntax sugar can be added later.
I'm considering adding
summary(dict)
syntax (dict without section) for the case you want to skip the section name, as systemd does. And maybesummary(section, key, value)
in the case you want to add key/value one by one. And while at it,summary(key, value)
in the case you want to add key/value one by one without a section. But those syntax sugar can be added later.
Added all those variants now, in a separate commit in case we prefer merge the initial commit first.
I also wanted a mode where booleans would turn into YES/NO in green/red, but that can be future improvement.
Done, with bool_yn
keyword argument.
Very pretty ;)
@dcbaker @jpakkane I think this is ready to merge now. I'm pretty happy with the improvements that I did in this last iteration.
@jpakkane, this looks good to me. Since it has frontend language changes I figured you'd like tot ake a peak before this lands.
This has been requested multiple times in the mesa/xorg community, and apparently in the GStreamer community as well.
This allows a configuration summary to be printed at the end of the configuring phase.
For example:
Which would print something like:
Fixes #757