Hi Pekka, It's great to see some tests for the scanner. There's a few thoughts I may have mentioned before - please don't read too much into them.
On 10 November 2016 at 09:57, Pekka Paalanen <[email protected]> wrote: > From: Pekka Paalanen <[email protected]> > > Add tests that ensure that wayland-scanner output for a given input does > not change unexpectedly. This makes it very easy to review > wayland-scanner patches. > > Before, when patches were proposed for wayland-scanner, I had to > build wayland without the patches, save the generated files into a > temporary directory, apply the patches, build again, and diff the old > vs. new generated file. > > No more. Now whenever someone makes intentional changes to > wayland-scanner's output, he will also have to patch the example output > files to match. That means that reviewers see the diff of the generated > files straight from the patch itself. Verifying the diff is true is as > easy as 'make check'. > > The tests use separate example XML files instead of wayland.xml > directly, so that wayland.xml can be updated without fixing scanner > tests, avoiding the churn. > > example.xml starts as a copy of wayland.xml. If wayland.xml starts using > new wayland-scanner features, they should be copied into example.xml > again to be covered by the tests. > That's the weakest point in the current proposal and something that should be addressed, imho. "The problem": wayland{,-scanner} has changed ABI in a non-backwards (or forward depending on the POV) manner a few times. Such that when built against the newer wayland{,-scanner} it won't work with older one. While that happens rarely, one should provide control, so that one can choose "use the new X code-paths" or not. This way today's implicit dependency will be transformed to explicit one. Obviously one can argue that we should use the same version of wayland at build and runtime. Yet binary distributions (or any distribution for that manner) do not limit upper version of the dependencies. Thus it's not uncommon to have scenario like the following: Package requires: vX [pulled of the configure error/warning] Build against: vX+1. Thus at run-time [with vX] the prebuild package will fail due to the implicit dependency for vX+1 during build time. To resolve that, I'm suggesting: a) example.xml must always be kept in sync with wayland.xml b) guards around new API should be used and propagated [in the generated files] by the scanner. #ifdef USE_NEW_API_FOO foo_new() #else foo() #endif The guards can be in two(or more?) forms - explicit per-feature/fix (USE_NEW API_*) or general I want all fixes (USE_WAYLAND_API_*). Both approaches have positive and negative sides. c) make check can be deployed to establish if the API used matches the user preference. One way to do it is to have a dummy binary before/after each API/ABI break thus one can parse the API pulled/used by it via nm, objdump or alike. All that said, _none_ of the above is something that should be addressed at this stage. Although it would be great to happen before the next API/ABI break ;-) Thanks Emil _______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
