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.
>
>
> 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
>
>
> 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?
> host_os:linux1: but not work in my opinion looks strange
> var1 = 2
Without named variants, this:
linux1:
var1 = 2
already works, right?
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 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?
> 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 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.
>
> 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
