| Message ID | 1421881250.13638.10.camel@x220 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/21/15 15:00, Paul Bolle wrote: > rev_dep expressions can get rather unwieldy, especially if a symbol is > selected by more than a handful of other symbols. Ie, it's possible to > have near endless expressions like: > A && B && !C || D || F && (G || H) || [...] > > Chop these expressions into actually readable chunks: > - A && B && !C > - D > - F && (G || H) > - [...] > > Ie, transform the top level "||" tokens into newlines and prepend each > line with a minus. This makes the "Selected by:" blurb much easier to > read. I agree that something like this needs to be done. After reading these expressions for a few years, I can handle them OK, but I expect that I look at them more than most people do (with the exception of Paul). Here is another confusing one, at least to me: [This is for LBDAF on a 64-biy x86 kernel config.] Depends on: BLOCK [=y] && !64BIT [=y] I guess that the value [y,m,n] inside the square brackets is always(?) the symbol value (64BIT) and not the expression value (!64BIT), but that isn't always clear IMO. > Not-yet-signed-off-by: Paul Bolle <pebolle@tiscali.nl> > --- > Today I found myself wondering why a certain Kconfig was selected. > Currently menuconfig's help is of no use in complicated cases. Please > look at the help of USB or CRYPTO to see what I mean. > > This is a _hack_ to show what might be a better way to do this. It > parses a stringified version of the reverse dependency, and not the > actual reverse dependecy expression. But that was easier to cobble > together. > > One cool improvement would be to change to minus in front of the > subexpressions to Y or M for those that actually set the symbol. Anyhow, > other suggestions and feedback is welcome. > > scripts/kconfig/menu.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 81 insertions(+), 2 deletions(-)
On Wed, 2015-01-21 at 18:23 -0800, Randy Dunlap wrote: > On 01/21/15 15:00, Paul Bolle wrote: > > rev_dep expressions can get rather unwieldy, especially if a symbol is > > selected by more than a handful of other symbols. Ie, it's possible to > > have near endless expressions like: > > A && B && !C || D || F && (G || H) || [...] > > > > Chop these expressions into actually readable chunks: > > - A && B && !C > > - D > > - F && (G || H) > > - [...] > > > > Ie, transform the top level "||" tokens into newlines and prepend each > > line with a minus. This makes the "Selected by:" blurb much easier to > > read. > > I agree that something like this needs to be done. > > After reading these expressions for a few years, I can handle them OK, but > I expect that I look at them more than most people do (with the exception of Paul). That's a problem with a broader scope. The specific problem this RFC targets is neatly demonstrated by this screenshot: Symbol: SERIAL_CORE_CONSOLE [=n] Type : boolean Defined at drivers/tty/serial/Kconfig:805 Depends on: TTY [=n] && HAS_IOMEM [=y] Selected by: SERIAL_8250_CONSOLE [=n] && TTY [=n] && HAS_IOMEM [=y] && SERIAL_8250 [=n]=y || SERIAL_AMBA_PL010_CONSOLE [=n] && [...] That line ends about two meters to the right of your screen. Really. > Here is another confusing one, at least to me: > [This is for LBDAF on a 64-biy x86 kernel config.] > > Depends on: BLOCK [=y] && !64BIT [=y] > > I guess that the value [y,m,n] inside the square brackets is always(?) the > symbol value (64BIT) and not the expression value (!64BIT), but that isn't > always clear IMO. I think you got that right. I guess the entire screen for LBDAF looked like this _hand edited_ example: Symbol: LBDAF [=n] Type : boolean Prompt: Support for large (2TB+) block devices and files Location: (1) -> Enable the block layer (BLOCK [=y]) Defined at block/Kconfig:26 Depends on: BLOCK [=y] && !64BIT [=y] In this case 64BIT is set to 'y' (otherwise LBDAF would have been 'y'). This isn't a bug issue, of course, but I still can see how this can be confusing. Perhaps the last line should read: Unmet dependency on: BLOCK [=y] && !64BIT [=y] Would that help? Or would Depends on: BLOCK [=y] && !64BIT [=n] (ie, print the value if "!64BIT") be clearer? > > Not-yet-signed-off-by: Paul Bolle <pebolle@tiscali.nl> I'm glad I added that! > > Today I found myself wondering why a certain Kconfig was selected. > > Currently menuconfig's help is of no use in complicated cases. Please > > look at the help of USB or CRYPTO to see what I mean. > > > > This is a _hack_ to show what might be a better way to do this. It > > parses a stringified version of the reverse dependency, and not the > > actual reverse dependecy expression. But that was easier to cobble > > together. > > > > One cool improvement would be to change to minus in front of the > > subexpressions to Y or M for those that actually set the symbol. Anyhow, > > other suggestions and feedback is welcome. > > > > scripts/kconfig/menu.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++-- > > 1 file changed, 81 insertions(+), 2 deletions(-) Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2015-01-22 at 00:00 +0100, Paul Bolle wrote: > rev_dep expressions can get rather unwieldy, especially if a symbol is > selected by more than a handful of other symbols. Ie, it's possible to > have near endless expressions like: > A && B && !C || D || F && (G || H) || [...] > > Chop these expressions into actually readable chunks: > - A && B && !C > - D > - F && (G || H) > - [...] > > Ie, transform the top level "||" tokens into newlines and prepend each > line with a minus. This makes the "Selected by:" blurb much easier to > read. > > Not-yet-signed-off-by: Paul Bolle <pebolle@tiscali.nl> > --- > Today I found myself wondering why a certain Kconfig was selected. > Currently menuconfig's help is of no use in complicated cases. Please > look at the help of USB or CRYPTO to see what I mean. > > This is a _hack_ to show what might be a better way to do this. It > parses a stringified version of the reverse dependency, and not the > actual reverse dependecy expression. But that was easier to cobble > together. > > One cool improvement would be to change to minus in front of the > subexpressions to Y or M for those that actually set the symbol. Anyhow, > other suggestions and feedback is welcome. > > scripts/kconfig/menu.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 81 insertions(+), 2 deletions(-) > > diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c > index 72c9dba84c5d..eb73fe77513e 100644 > --- a/scripts/kconfig/menu.c > +++ b/scripts/kconfig/menu.c > @@ -613,6 +613,86 @@ static struct property *get_symbol_prop(struct symbol *sym) > } > > /* > + * Assuming we're just past an opening parenthesis in a NUL terminated string, > + * find it's closing parenthesis and return its postion. Die otherwise. > + */ > +static const char *matching_paren(const char *s) > +{ > + int lvl = 1; > + > + while (1) { > + if (*s == '(') > + lvl++; > + else if (*s == ')') > + lvl--; > + if (lvl == 0) > + break; > + if (*s == '\0') > + /* huh? */ > + exit(1); > + s++; > + } > + > + return s; > +} > + > +/* > + * rev_dep expressions can get rather unwieldy, especially if a symbol is > + * selected by more than a handful of other symbols. Ie, it's possible to > + * have near endless expressions like: > + * A && B && !C || D || F && (G || H) || [...] > + * > + * Chop these expressions into actually readable chunks: > + * - A && B && !C > + * - D > + * - F && (G || H) > + * - [...] > + * > + * Ie, transform the top level "||" tokens into newlines and prepend each line > + * with a minus. This makes the "Selected by:" blurb much easier to read. > + */ > +static void rev_dep_gstr_print(struct gstr *gs, struct expr *e) Charge: posting just before suspending machine and self. Verdict: guilty. That should have been static void rev_dep_gstr_print(struct expr *e, struct gstr *gs) > +{ > + struct gstr tmp = str_new(); > + const char *prev, *start; > + char *beam; > + > + expr_gstr_print(e, &tmp); > + prev = start = str_get(&tmp); > + > + str_append(gs, "\n - "); > + > + while ((beam = index(start, '|'))) { > + char *lparen = index(start, '('); > + > + /* don't split "(I || J)" */ > + if (lparen && (lparen < beam)) { > + const char *rparen = matching_paren(++lparen); > + > + /* skip the expression inside parentheses */ > + start = ++rparen; > + continue; > + } > + > + /* we can assume we're fed a sane string, so the space before > + * the beam gets turned into a NUL */ > + *(beam - 1) = '\0'; > + str_append(gs, prev); > + str_append(gs, "\n - "); > + /* assume sane string, so skip the second beam */ > + beam++; > + /* trim */ > + while (*++beam == ' ') > + ; > + prev = start = beam; > + } > + > + str_append(gs, prev); > + > + str_free(&tmp); > +} > + > +/* > * head is optional and may be NULL > */ > void get_symbol_str(struct gstr *r, struct symbol *sym, > @@ -661,8 +741,7 @@ void get_symbol_str(struct gstr *r, struct symbol *sym, > str_append(r, "\n"); > if (sym->rev_dep.expr) { > str_append(r, _(" Selected by: ")); > - expr_gstr_print(sym->rev_dep.expr, r); > - str_append(r, "\n"); > + rev_dep_gstr_print(sym->rev_dep.expr, r); > } > str_append(r, "\n\n"); > } Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2015-01-22 at 09:35 +0100, Paul Bolle wrote: > On Wed, 2015-01-21 at 18:23 -0800, Randy Dunlap wrote: > > On 01/21/15 15:00, Paul Bolle wrote: > > > rev_dep expressions can get rather unwieldy, especially if a symbol is > > > selected by more than a handful of other symbols. Ie, it's possible to > > > have near endless expressions like: > > > A && B && !C || D || F && (G || H) || [...] > > > > > > Chop these expressions into actually readable chunks: > > > - A && B && !C > > > - D > > > - F && (G || H) > > > - [...] > > > > > > Ie, transform the top level "||" tokens into newlines and prepend each > > > line with a minus. This makes the "Selected by:" blurb much easier to > > > read. > > > > I agree that something like this needs to be done. > > > > After reading these expressions for a few years, I can handle them OK, but > > I expect that I look at them more than most people do (with the exception of Paul). > > That's a problem with a broader scope. The specific problem this RFC > targets is neatly demonstrated by this screenshot: > Symbol: SERIAL_CORE_CONSOLE [=n] > Type : boolean > Defined at drivers/tty/serial/Kconfig:805 > Depends on: TTY [=n] && HAS_IOMEM [=y] > Selected by: SERIAL_8250_CONSOLE [=n] && TTY [=n] && HAS_IOMEM [=y] && SERIAL_8250 [=n]=y || SERIAL_AMBA_PL010_CONSOLE [=n] && [...] > > That line ends about two meters to the right of your screen. Really. > > > Here is another confusing one, at least to me: > > [This is for LBDAF on a 64-biy x86 kernel config.] > > > > Depends on: BLOCK [=y] && !64BIT [=y] > > > > I guess that the value [y,m,n] inside the square brackets is always(?) the > > symbol value (64BIT) and not the expression value (!64BIT), but that isn't > > always clear IMO. > > I think you got that right. I guess the entire screen for LBDAF looked > like this _hand edited_ example: > Symbol: LBDAF [=n] > Type : boolean > Prompt: Support for large (2TB+) block devices and files > Location: > (1) -> Enable the block layer (BLOCK [=y]) > Defined at block/Kconfig:26 > Depends on: BLOCK [=y] && !64BIT [=y] > > In this case 64BIT is set to 'y' (otherwise LBDAF would have been 'y'). This is not correct at all. If 64BIT == 'n', LBDAF can be both 'y' or 'n'. Where's that brown paper bag again? > This isn't a bug issue, of course, but I still can see how this can be > confusing. Perhaps the last line should read: > Unmet dependency on: BLOCK [=y] && !64BIT [=y] > > Would that help? Or would > Depends on: BLOCK [=y] && !64BIT [=n] > > (ie, print the value if "!64BIT") be clearer? > > > > Not-yet-signed-off-by: Paul Bolle <pebolle@tiscali.nl> > > I'm glad I added that! > > > > Today I found myself wondering why a certain Kconfig was selected. > > > Currently menuconfig's help is of no use in complicated cases. Please > > > look at the help of USB or CRYPTO to see what I mean. > > > > > > This is a _hack_ to show what might be a better way to do this. It > > > parses a stringified version of the reverse dependency, and not the > > > actual reverse dependecy expression. But that was easier to cobble > > > together. > > > > > > One cool improvement would be to change to minus in front of the > > > subexpressions to Y or M for those that actually set the symbol. Anyhow, > > > other suggestions and feedback is welcome. > > > > > > scripts/kconfig/menu.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++-- > > > 1 file changed, 81 insertions(+), 2 deletions(-) Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2015-01-22 09:35, Paul Bolle wrote: > In this case 64BIT is set to 'y' (otherwise LBDAF would have been 'y'). > This isn't a bug issue, of course, but I still can see how this can be > confusing. Perhaps the last line should read: > Unmet dependency on: BLOCK [=y] && !64BIT [=y] > > Would that help? Or would > Depends on: BLOCK [=y] && !64BIT [=n] > > (ie, print the value if "!64BIT") be clearer? How about Depends on: (BLOCK [=y] && !64BIT [=y]) [=n] ? Michal -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2015-01-22 at 11:33 +0100, Michal Marek wrote: > On 2015-01-22 09:35, Paul Bolle wrote: > > In this case 64BIT is set to 'y' (otherwise LBDAF would have been 'y'). > > This isn't a bug issue, of course, but I still can see how this can be > > confusing. Perhaps the last line should read: > > Unmet dependency on: BLOCK [=y] && !64BIT [=y] > > > > Would that help? Or would > > Depends on: BLOCK [=y] && !64BIT [=n] > > > > (ie, print the value if "!64BIT") be clearer? > > How about > > Depends on: (BLOCK [=y] && !64BIT [=y]) [=n] > > ? Or Depends on: BLOCK [=y] && !64BIT [=y] => [=n] Whatever, we'll figure out something. This is a curses UI, isn't it? Could we use color to distinguish the symbols or sub-expressions that are set correctly, for that particular dependency, from those that are not? Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2015-01-22 11:47, Paul Bolle wrote: > On Thu, 2015-01-22 at 11:33 +0100, Michal Marek wrote: >> On 2015-01-22 09:35, Paul Bolle wrote: >>> In this case 64BIT is set to 'y' (otherwise LBDAF would have been 'y'). >>> This isn't a bug issue, of course, but I still can see how this can be >>> confusing. Perhaps the last line should read: >>> Unmet dependency on: BLOCK [=y] && !64BIT [=y] >>> >>> Would that help? Or would >>> Depends on: BLOCK [=y] && !64BIT [=n] >>> >>> (ie, print the value if "!64BIT") be clearer? >> >> How about >> >> Depends on: (BLOCK [=y] && !64BIT [=y]) [=n] >> >> ? > > Or > Depends on: BLOCK [=y] && !64BIT [=y] => [=n] > > Whatever, we'll figure out something. > > This is a curses UI, isn't it? Could we use color to distinguish the > symbols or sub-expressions that are set correctly, for that particular > dependency, from those that are not? That might be a bit tricky, since there are multiple UIs and we use the same functions to build the displayed strings. Michal -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c index 72c9dba84c5d..eb73fe77513e 100644 --- a/scripts/kconfig/menu.c +++ b/scripts/kconfig/menu.c @@ -613,6 +613,86 @@ static struct property *get_symbol_prop(struct symbol *sym) } /* + * Assuming we're just past an opening parenthesis in a NUL terminated string, + * find it's closing parenthesis and return its postion. Die otherwise. + */ +static const char *matching_paren(const char *s) +{ + int lvl = 1; + + while (1) { + if (*s == '(') + lvl++; + else if (*s == ')') + lvl--; + if (lvl == 0) + break; + if (*s == '\0') + /* huh? */ + exit(1); + s++; + } + + return s; +} + +/* + * rev_dep expressions can get rather unwieldy, especially if a symbol is + * selected by more than a handful of other symbols. Ie, it's possible to + * have near endless expressions like: + * A && B && !C || D || F && (G || H) || [...] + * + * Chop these expressions into actually readable chunks: + * - A && B && !C + * - D + * - F && (G || H) + * - [...] + * + * Ie, transform the top level "||" tokens into newlines and prepend each line + * with a minus. This makes the "Selected by:" blurb much easier to read. + */ +static void rev_dep_gstr_print(struct gstr *gs, struct expr *e) +{ + struct gstr tmp = str_new(); + const char *prev, *start; + char *beam; + + expr_gstr_print(e, &tmp); + prev = start = str_get(&tmp); + + str_append(gs, "\n - "); + + while ((beam = index(start, '|'))) { + char *lparen = index(start, '('); + + /* don't split "(I || J)" */ + if (lparen && (lparen < beam)) { + const char *rparen = matching_paren(++lparen); + + /* skip the expression inside parentheses */ + start = ++rparen; + continue; + } + + /* we can assume we're fed a sane string, so the space before + * the beam gets turned into a NUL */ + *(beam - 1) = '\0'; + str_append(gs, prev); + str_append(gs, "\n - "); + /* assume sane string, so skip the second beam */ + beam++; + /* trim */ + while (*++beam == ' ') + ; + prev = start = beam; + } + + str_append(gs, prev); + + str_free(&tmp); +} + +/* * head is optional and may be NULL */ void get_symbol_str(struct gstr *r, struct symbol *sym, @@ -661,8 +741,7 @@ void get_symbol_str(struct gstr *r, struct symbol *sym, str_append(r, "\n"); if (sym->rev_dep.expr) { str_append(r, _(" Selected by: ")); - expr_gstr_print(sym->rev_dep.expr, r); - str_append(r, "\n"); + rev_dep_gstr_print(sym->rev_dep.expr, r); } str_append(r, "\n\n"); }