Hi,<div>
<div class="gmail_extra">On 2 November 2012 14:37, Peter Hutterer <span dir="ltr"><<a href="mailto:peter.hutterer@who-t.net" target="_blank">peter.hutterer@who-t.net</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div>can you covert your test to a XIT test and add it to (a newly created)<br></div>
tests/server/xkb?<br>
<a href="http://cgit.freedesktop.org/~whot/xorg-integration-tests/" target="_blank">http://cgit.freedesktop.org/~whot/xorg-integration-tests/</a></blockquote><div><br></div><div>Are these tests ever supposed to work with anything other than master? I wrote a test which I'll send to the list, but if I push that, the tests are going to fail on literally every released version of the server ever, which doesn't sound particularly useful.</div>
<div><br></div><div>Is there much of a compelling reason for not having the tests in-tree with the server? What's the actual split between xorg-gtest and xorg-integration-tests? Why does the former have a couple of random tests and a device declaration file if it seems to mainly just be a framework? Is there much use in having the split, given how codependent they seem to be?</div>
<div><br></div><div>Also, it relies on evemu, which isn't packaged in Debian, which is kind of irritating. (Also, evemu's autogen.sh isn't actually the standard X.Org autogen.sh, and doesn't run ./configure for you. And it's on Launchpad, using Bazaar ... I die. For bonus points, it's even licensed under GPLv3, which means a hell of a lot of companies will not go near it no matter what, and probably just skip the integration tests.)</div>
<div><br></div><div>With all the linking of massive chunks of C++, it's also crazy slow to build: I can autoreconf, configure, make, and make check Weston and its sample clients from scratch at least four times over in the time it takes to just build the tests, let alone run them. Probably closer to five or six. When I do get to run them, the Synaptics tests (massively valuable, don't get me wrong) all interfere with my running server, presumably because it doesn't know to ignore a test device. That's something that'd be really nice to have a flag for. The Wacom tests are all failing (presumably due to not having the driver installed), but between the two-second delay for server starting and five-second device wait, are taking seven seconds a pop. There's forty-eight of those tests, which makes it just over five and a half minutes. I also now have a bajillion files directly under /tmp.</div>
<div><br></div><div>The keyboard tests all failed because the dummy display driver configuration didn't get written out, so it tried to spawn a new Intel server, only to die when it realised it couldn't become a DRM master.</div>
<div><br></div><div>Sorry if this comes across as kind of harsh - I really, really like the idea, but it comes across as pretty irritating to actually write the tests. Take this with a grain of salt since I barely touch X these days, but it just seems way too clunky become part of my regular workflow. It's all fixable, mind (with the possible exception of the massive chunks o' C++), but the out-of-the-box experience isn't much fun.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div>> - list.pattern[_XkbListKeycodes] = GetComponentSpec(&str, FALSE, &status);<br>
> - list.pattern[_XkbListTypes] = GetComponentSpec(&str, FALSE, &status);<br>
> - list.pattern[_XkbListCompat] = GetComponentSpec(&str, FALSE, &status);<br>
> - list.pattern[_XkbListSymbols] = GetComponentSpec(&str, FALSE, &status);<br>
> - list.pattern[_XkbListGeometry] = GetComponentSpec(&str, FALSE, &status);<br>
> - if (status != Success)<br>
> - return status;<br>
> + for (i = 0; i < _XkbListNumComponents; i++) {<br>
> + size = *((uint8_t *)str);<br>
> + str += (size + 1);<br>
> + }<br>
<br>
</div></div>please add a short comment that str is a set of pascal-style strings so<br>
we're really just skipping over the content of the string here. it's not<br>
really obvious for the unsuspecting reviewer.<br></blockquote><div><br></div><div>Sure, I've done this now. I've also changed the 5 to 6: turns out that 1ad80678 (committed 2009) broke this function anyway. It shifted all the component masks up one (so your keycode spec was actually the keymap spec, types were keycodes, etc), and it'd also give you BadLength unless you specified * for everything, or otherwise managed to have your length such that the length of keymap string spec + 1 fit into the four-character padding.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
I suspect if anyone was really using this instead of the xml, we won't hear<br>
of it until this feeds into RHEL or a similar distro...<br></blockquote><div><br></div><div>Given that's the second problem I've found with this code, I doubt it'll be too much of an issue.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
anyway, with the comment:<br>
Reviewed-by: Peter Hutterer <<a href="mailto:peter.hutterer@who-t.net" target="_blank">peter.hutterer@who-t.net</a>><br></blockquote><div><br></div><div>Thanks, resent if you want to pull into your tree.</div><div>
<br></div><div>Cheers,</div>
<div>Daniel </div></div></div></div>