----- Original Message -----
> On Thu, Apr 04, 2013 at 02:08:48PM -0400, Jiri Zupka wrote:
> > Hi Eduardo,
> >
> > what do you say?
> >
> > default
> > variants guest_os: expand all variants
> > - linux1:
> > - linux2:
> >
> > variants host_os: take linux1
> > - @linux1:
> > - linux2:
>
> I like this mode of operation. I will refer to it as "non-agressive
> expansion mode". In other words: expand everything if there's no "@",
> and use the "@" option if there's only one.
>
> The problem is: if we make non-agressive-expansion mode the default, we
> will break compartibility with existing config files/jobs where people
> expect everything to be expanded by default.
>
> But even if we don't make it the default, it would be nice to have it
> very easy to enable, as it makes sense for many use cases.
I agree with easy enabling and disabling default. In example above missing non
default mode.
Already implemented.
>
> >
> >
> > First part as we talk before.
> >
> >
> > Double default variant:
> >
> > variants system: It should be impossible at all? There is
> > some case which uses it.
> > - @ asaa Or there should be other marker? I think and
> > hope that new marker
> > - @ aaaa will not be necessary but with new maker it
> > could be cleaner.
>
> That's a good question. It's all about what makes sense to have enabled
> by default or in a easy-to-enable way. I think I think the most
> intuitive behavior is to simply expand only the "@" items by default if
> we are on non-agressive-expansion mode.
>
> Also, I would like to have other ways to mark the default options. "@"
> means "hidden" (not appearing in short_name). It is useful to use it
> because it is short and makes sense on many cases, but it would be nice
> to allow the default variant to be chosen without requiring it to be
> "hidden".
>
> Once we decide to have an extensible syntax to set metadata on
> variants-blocks, we could use it to set the default variant and
> expansion-mode, too. e.g.:
>
> variants system [default=aaa]:
> - aaa
> - bbb
>
> variants subtest [expand_all=true]:
> - test1
> - test2
>
It could help for future syntax expanding. As Paolo mentioned on github.
And make configuration more flexible for users.
> >
> >
> > 1) Exchange '>' to '='
> >
> > only host_os=linux2 should work
> > host_os=linux: should be condition not assigning value to
> > variable
> >
> > then there will be impossible do assigning:
> > var1=sddd: not work nobody know if this is condition or
> > something else.
> > var2="sddd:" should work
>
> Oh, so the problem is the ambiguity when "=" is used on conditional
> "<condition>:" blocks. Now I see.
>
> >
> > 2) Exchange '>' to ':'
> >
> > only host_os:linux2 should work
> > host_os:linux1: var = 2 could work but not good visible
>
> Without named variants, "linux1: var = 2" already works, right?
sorry yes, copy paste.. but looks strange.
>
> > host_os:linux1: but not work in my opinion looks
> > strange
> > var1 = 2
>
> Without named variants, this:
>
> linux1:
> var1 = 2
>
> already works, right?
yes
>
> I think ":" would work, but it looks strange because the final ":" has a
> different meaning from the ":" in the middle.
>
> >
> > 3) Exchange '>' to '::'
> > little more performance expensive
>
> Why exactly "::" would have any performance problem?
only comparison of char and multiple char and more one if many times.
>
>
> > only host_os::linux2 should work
> > host_os::linux1: var = 2 could work but
> > host_os::linux1: work but
> > var1 = 2
> >
> > 4) Exchange '>' to '-'
> >
> > only host_os-linux2 should work
> > host_os-linux1: var = 2 could work but not good visible
>
> What "not good visible" means?
If you have lots of
host_os-guest1.virt_os-linux.sdff-aaaaa.
Then it could be hard to see in first sight in logs. could be hidden in text.
And as you wrote before looks like part of name.
>
> > host_os-linux1:
> > var1 = 2
> >
> > Which of this version do you prefer most or do you prefer some different
> > version?
>
> So, the main problem here seems to be how to make the "<condition>:"
> blocks look good and intuitive.
>
> The answer is: I don't know! I need to think about it a little.
> Currently I see two main possible approaches:
>
> * The ones you proposed above (Use something else than "=").
> I believe any of ":", "::" or "-" look better than ">". I think I
> would choose "::". ":" is confusing (see my comment above), "-"
> doesn't look like an "operator", but just part of the variant name.
> * Require some kind of delimiter when "=" is involved. e.g.:
> only [host_os=linux2]
> [host_os=linux2]: var = 2
> [host_os=linux2]:
> var = 2
> [host_os == "linux2"]: var = 2
>
> I would love to hear suggestions from others as well. Paolo?
I like this version. It could be easy and fast for parsing. And good visible.
only [host_os=linux2]
[host_os=linux2]: var = 2
[host_os=linux2]:
var = 2
disadvantage: This breaks today syntax. There could be choice
for named variant and filtering with name in config have to be
only [host_os=linux2]
for unnamed it could stay as it for keep compatibility.
only linux2
but could be replaced by
only [linux2]
only [host_os=linux2.guest_os]
or
only linux2.guest_os
host_os=linux2.guest_os: this will be interpreted like variable
assignation.
[host_os=linux2.guest_os]: condition.
It could be little confusing for users..
This version is quite overkill from my point of view.
[host_os == "linux2"]: var = 2
I tried to ask person which never see this syntax how they understand
host_os::linux2 and [host_os=linux2]
and they choosed [host_os=linux2] as more intuitive understandable.
>
> >
> >
> > I like your idea with expand_variants. Which with "run" it is not necessary
> > implement it to
> > cart config directly for now. But for autoserv tests it could be better.
> >
> > > # will expand only the "subtest" and "guest_os" variants-block, and
> > > # automatically choose defaults for every other variants-block:
> > > c = Parser(..., expand_variants=['subtest', 'guest_os'])
> > > # For somebody who wants to run the default tests with all Windows
> > > # versions:
> > > c = Parser(..., expand_variants=['subtest', 'windows_version'])
> > > # For somebody who wants to run the default tests with all CPU models,
> > > # and all guest OSes:
> > > c = Parser(..., expand_variants=['subtest', 'guest_os', 'cpu_model'])
> > > # (additional nice-to-have: to allow something like "guest_os.*" to
> > > # expand a variants-block and all its "sub-variants" (guest OS
> > > # version, guest OS architecture, etc)
> >
> >
> > It is necessary to solve one case where there is unnamed variant with
> > default.
> > Then it impossible to expand unnamed variant with expand_variants=[].
> > By this reason the selection of default variant shouldn't work in this
> > case even if variant name contain @.
> >
> > example:
> > variants: expand all variant again.
> > - @first:
> > var1= xxxxx
> > - second
>
> We can make the "@"-by-default case work internally even on unnamed
> blocks, we don't need to make all the filtering/expanding decisions be
> solely based on string parameters. The string parameters are just a way
> to choose a parser/expander behavior that's different from the default.
>
> Now, if an user wants to be able to manually enable/disable expansion of
> an unnamed variants-block from the command-line or control file, then
> they should add a name to the block to be able to do it. I don't think
> it is a big problem.
Ok, make sense.
>
> >
> > Thanks for review and your time:-)
> > Jiří
> >
> > ----- Original Message -----
> > > On Fri, Mar 29, 2013 at 06:14:09PM +0100, Jiří Župka wrote:
> > > > If default variant is not filtered by only or no filters then
> > > > only default variant is chosen. This behavior was used for optimizing
> > > > of speed of Cartesian config.
> > > > If variants don't have default variant then everything works as usual.
> > > > Default variant must be in variants with with_default exactly one
> > > > times.
> > > > The default variant could be filtered by only, no filter. If default
> > > > variant is filtered from variants then variants works same as usual
> > > > variants with
> > > > default variant.
> > > >
> > > > For calling Cartesian config from command line is used option
> > > > -d/--defaults:
> > > > ../virttest/cartesian_config.py -d cfg/cc.cfg
> > > >
> > > > For calling Cartesian config from python:
> > > > c = Parser(args[0], defaults=options.defaults, debug=options.debug)
> > >
> > > I believe we have allow that to be per-variants-block, not
> > > all-or-nothing.
> > >
> > > For example: the default ./run behavior could be to automatically choose
> > > defaults for every variants-block (guest OS, block format, etc), except
> > > for the "subtests" variants-block.
> > >
> > > I would like the API to look like this:
> > >
> > > # this would be the current behavior:
> > > c = Parser(..., expand_all_variants=True)
> > > # will expand only the "subtest" and "guest_os" variants-block, and
> > > # automatically choose defaults for every other variants-block:
> > > c = Parser(..., expand_variants=['subtest', 'guest_os'])
> > > # For somebody who wants to run the default tests with all Windows
> > > # versions:
> > > c = Parser(..., expand_variants=['subtest', 'windows_version'])
> > > # For somebody who wants to run the default tests with all CPU models,
> > > # and all guest OSes:
> > > c = Parser(..., expand_variants=['subtest', 'guest_os', 'cpu_model'])
> > > # (additional nice-to-have: to allow something like "guest_os.*" to
> > > # expand a variants-block and all its "sub-variants" (guest OS
> > > # version, guest OS architecture, etc)
> > >
> > > We could also find a way to encode the "expand_variants" instruction
> > > inside the config file syntax, so people could put that information in
> > > their config file. But that can be done later, after we test if the
> > > concept is really working in the Python API and command-line.
> > >
> > > >
> > > > ********* example:
> > > > variants name=tests:
> > > > - wait:
> > > > run = "wait"
> > > > variants:
> > > > - long:
> > > > time = short_time
> > > > - short: long
> > > > time = logn_time
> > > > - test2:
> > > > run = "test1"
> > > >
> > > > variants name=virt_system, with_default:
> > > > - @linux:
> > > > - windows:
> > > >
> > > > variants name=host_os, with_default:
> > > > - linux:
> > > > image = linux
> > > > variants with_default:
> > > > - ubuntu:
> > > > - @fedora:
> > > > - windows:
> > > > image = windows
> > > > variants:
> > > > - @XP:
> > > > - WIN7:
> > > >
> > > > only host_os>windows
> > > >
> > > > In this case is chosen from host_os variants windows variant.
> > > > host_os>windows was choosen because default variant linux was filtered.
> > > > Next step is select one variant from guest_os. There will be chosen
> > > > only
> > > > default variant linux because not filtered and virt_system variant is
> > > > with with_default. There is no default variant in tests variants
> > > > because
> > > > that all of tests will be chosen.
> > > >
> > > > ******** output:
> > > > dict 1: host_os>windows.tests>wait.long
> > > > dep = []
> > > > host_os = windows
> > > > image = windows
> > > > name = host_os>windows.XP.virt_system>linux.tests>wait.long
> > > > run = wait
> > > > shortname = host_os>windows.tests>wait.long
> > > > tests = wait
> > > > time = short_time
> > > > virt_system = linux
> > > > dict 2: host_os>windows.tests>wait.short
> > > > dep = ['host_os>windows.XP.virt_system>linux.tests>wait.long']
> > > > host_os = windows
> > > > image = windows
> > > > name = host_os>windows.XP.virt_system>linux.tests>wait.short
> > > > run = wait
> > > > shortname = host_os>windows.tests>wait.short
> > > > tests = wait
> > > > time = logn_time
> > > > virt_system = linux
> > > > dict 3: host_os>windows.tests>test2
> > > > dep = []
> > > > host_os = windows
> > > > image = windows
> > > > name = host_os>windows.XP.virt_system>linux.tests>test2
> > > > run = test1
> > > > shortname = host_os>windows.tests>test2
> > > > tests = test2
> > > > virt_system = linux
> > > > dict 4: host_os>windows.WIN7.tests>wait.long
> > > > dep = []
> > > > host_os = windows
> > > > image = windows
> > > > name = host_os>windows.WIN7.virt_system>linux.tests>wait.long
> > > > run = wait
> > > > shortname = host_os>windows.WIN7.tests>wait.long
> > > > tests = wait
> > > > time = short_time
> > > > virt_system = linux
> > > > dict 5: host_os>windows.WIN7.tests>wait.short
> > > > dep = ['host_os>windows.WIN7.virt_system>linux.tests>wait.long']
> > > > host_os = windows
> > > > image = windows
> > > > name = host_os>windows.WIN7.virt_system>linux.tests>wait.short
> > > > run = wait
> > > > shortname = host_os>windows.WIN7.tests>wait.short
> > > > tests = wait
> > > > time = logn_time
> > > > virt_system = linux
> > > > dict 6: host_os>windows.WIN7.tests>test2
> > > > dep = []
> > > > host_os = windows
> > > > image = windows
> > > > name = host_os>windows.WIN7.virt_system>linux.tests>test2
> > > > run = test1
> > > > shortname = host_os>windows.WIN7.tests>test2
> > > > tests = test2
> > > > virt_system = linux
> > > >
> > > > Signed-off-by: Jiří Župka <[email protected]>
> > > > ---
> > > > virttest/cartesian_config.py | 82
> > > > ++++++++++++++++++++++++++++++++++++--------
> > > > 1 file changed, 68 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/virttest/cartesian_config.py
> > > > b/virttest/cartesian_config.py
> > > > index 04ed2b5..6cd0e88 100755
> > > > --- a/virttest/cartesian_config.py
> > > > +++ b/virttest/cartesian_config.py
> > > > @@ -131,6 +131,10 @@ class ParserError:
> > > > return "%s (%s:%s)" % (self.msg, self.filename,
> > > > self.linenum)
> > > >
> > > >
> > > > +class MissingDefault:
> > > > + pass
> > > > +
> > > > +
> > > > class MissingIncludeError:
> > > > def __init__(self, line, filename, linenum):
> > > > self.line = line
> > > > @@ -225,6 +229,7 @@ class Node(object):
> > > > self.labels = set()
> > > > self.append_to_shortname = False
> > > > self.failed_cases = collections.deque()
> > > > + self.default = False
> > > >
> > > >
> > > > def dump(self, indent, recurse=False):
> > > > @@ -407,14 +412,17 @@ class Parser(object):
> > > >
> > > > @see:
> > > >
> > > > https://github.com/autotest/autotest/wiki/KVMAutotest-CartesianConfigParametersIntro
> > > > """
> > > > - def __init__(self, filename=None, debug=False):
> > > > + def __init__(self, filename=None, defaults=False, debug=False):
> > > > """
> > > > Initialize the parser and optionally parse a file.
> > > >
> > > > @param filename: Path of the file to parse.
> > > > + @param defaults: If True adds only defaults variant from
> > > > variants
> > > > + if there is some.
> > > > @param debug: Whether to turn on debugging output.
> > > > """
> > > > self.node = Node()
> > > > + self.defaults = defaults
> > > > self.debug = debug
> > > > if filename:
> > > > self.parse_file(filename)
> > > > @@ -611,10 +619,18 @@ class Parser(object):
> > > >
> > > > # Recurse into children
> > > > count = 0
> > > > - for n in node.children:
> > > > - for d in self.get_dicts(n, ctx, new_content, shortname,
> > > > dep):
> > > > - count += 1
> > > > - yield d
> > > > + if self.defaults:
> > > > + for n in node.children:
> > > > + for d in self.get_dicts(n, ctx, new_content,
> > > > shortname,
> > > > dep):
> > > > + count += 1
> > > > + yield d
> > > > + if n.default and count:
> > > > + break
> > > > + else:
> > > > + for n in node.children:
> > > > + for d in self.get_dicts(n, ctx, new_content,
> > > > shortname,
> > > > dep):
> > > > + count += 1
> > > > + yield d
> > > > # Reached leaf?
> > > > if not node.children:
> > > > self._debug(" reached leaf, returning it")
> > > > @@ -656,7 +672,8 @@ class Parser(object):
> > > > print s % args
> > > >
> > > >
> > > > - def _parse_variants(self, cr, node, prev_indent=-1,
> > > > var_name=None):
> > > > + def _parse_variants(self, cr, node, prev_indent=-1, var_name=None,
> > > > + with_default=False):
> > > > """
> > > > Read and parse lines from a FileReader object until a line
> > > > with an
> > > > indent level lower than or equal to prev_indent is
> > > > encountered.
> > > > @@ -665,8 +682,10 @@ class Parser(object):
> > > > @param node: A node to operate on.
> > > > @param prev_indent: The indent level of the "parent" block.
> > > > @param var_name: Variants name
> > > > + @param with_default: Variants take only default variant.
> > > > @return: A node object.
> > > > """
> > > > + already_default = False
> > > > node4 = Node()
> > > >
> > > > while True:
> > > > @@ -694,7 +713,9 @@ class Parser(object):
> > > > node2.labels = node.labels
> > > >
> > > > node3 = self._parse(cr, node2, prev_indent=indent)
> > > > - node3.name = [Label(var_name, n) for n in
> > > > name.lstrip("@").split(".")]
> > > > + is_default = name.startswith("@")
> > > > + name = name.lstrip("@")
> > > > + node3.name = [Label(var_name, n) for n in name.split(".")]
> > > > node3.dep = [Label(var_name, d) for d in dep.replace(",",
> > > > "
> > > > ").split()]
> > > >
> > > > if var_name:
> > > > @@ -702,12 +723,33 @@ class Parser(object):
> > > > op_match = _ops_exp.search(l)
> > > > node3.content += [(cr.filename, linenum, Op(l,
> > > > op_match))]
> > > >
> > > > - is_default = name.startswith("@")
> > > > -
> > > > - node4.children += [node3]
> > > > + node3.append_to_shortname = not is_default
> > > > +
> > > > + if with_default and self.defaults:
> > > > + """
> > > > + Relevant only if defaults is True and
> > > > + variants is with default.
> > > > + """
> > > > + if is_default:
> > > > + if not already_default:
> > > > + node3.default = True
> > > > + already_default = True
> > > > + else:
> > > > + raise MissingDefault
> > > > + if node3.default:
> > > > + # Move default variant in front of rest of all
> > > > variants.
> > > > + # Speed optimization.
> > > > + node4.children.insert(0, node3)
> > > > + else:
> > > > + node4.children += [node3]
> > > > + else:
> > > > + node4.children += [node3]
> > > > node4.labels.update(node3.labels)
> > > > node4.labels.update(node3.name)
> > > >
> > > > + if with_default and not already_default:
> > > > + raise MissingDefault
> > > > +
> > > > return node4
> > > >
> > > >
> > > > @@ -751,6 +793,7 @@ class Parser(object):
> > > > char in "._-=,"):
> > > > raise ParserError("Illegal characters in
> > > > variants",
> > > > line, cr.filename,
> > > > linenum)
> > > > + with_default = False
> > > > var_name = None
> > > > if name:
> > > > block = name.split(",")
> > > > @@ -761,12 +804,20 @@ class Parser(object):
> > > > raise ParserError("Missing name of
> > > > variants",
> > > > line, cr.filename,
> > > > linenum)
> > > > var_name = oper[1].strip()
> > > > + elif "with_default" in oper[0]:
> > > > + with_default = True
> > > > else:
> > > > raise ParserError("Ilegal variants param",
> > > > line, cr.filename,
> > > > linenum)
> > > > - node = self._parse_variants(cr, node,
> > > > prev_indent=indent,
> > > > - var_name=var_name)
> > > > - var_name=name)
> > > > + try:
> > > > + node = self._parse_variants(cr, node,
> > > > prev_indent=indent,
> > > > + var_name=var_name,
> > > > +
> > > > with_default=with_default)
> > > > + except MissingDefault:
> > > > + raise ParserError("There must be exactly one
> > > > default "
> > > > + "variant in variants with param
> > > > "
> > > > + "with_default.",
> > > > + line, cr.filename, linenum)
> > > > continue
> > > >
> > > > # Parse 'include' statements
> > > > @@ -1040,12 +1091,15 @@ if __name__ == "__main__":
> > > > help="show dict contents")
> > > > parser.add_option("-r", "--repr", dest="repr_mode",
> > > > action="store_true",
> > > > help="Output parsing results Python format")
> > > > + parser.add_option("-d", "--defaults", dest="defaults",
> > > > action="store_true",
> > > > + help="use only default variant of variants if
> > > > there"
> > > > + " is some")
> > > >
> > > > options, args = parser.parse_args()
> > > > if not args:
> > > > parser.error("filename required")
> > > >
> > > > - c = Parser(args[0], debug=options.debug)
> > > > + c = Parser(args[0], defaults=options.defaults,
> > > > debug=options.debug)
> > > > for s in args[1:]:
> > > > c.parse_string(s)
> > > >
> > > > --
> > > > 1.8.1.4
> > > >
> > >
> > > --
> > > Eduardo
> > >
>
> --
> Eduardo
>
_______________________________________________
Virt-test-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/virt-test-devel
