Den tis 30 juni 2026 kl 15:35 skrev Ivan Zhakov <[email protected]>: > On Sun, 28 Jun 2026 at 17:14, Daniel Sahlberg <[email protected]> > wrote: > >> Den tors 25 juni 2026 kl 15:57 skrev Ivan Zhakov <[email protected]>: >> >>> On Wed, 24 Jun 2026 at 18:03, Branko Čibej <[email protected]> wrote: >>> >>>> On 24. 6. 2026 16:03, Ivan Zhakov wrote: >>>> >>>> On Tue, 23 Jun 2026 at 17:42, Daniel Sahlberg < >>>> [email protected]> wrote: >>>> >>>>> Den tis 23 juni 2026 kl 15:34 skrev <[email protected]>: >>>>> >>>>>> Author: ivan >>>>>> Date: Tue Jun 23 13:33:58 2026 >>>>>> New Revision: 1935586 >>>>>> >>>>>> Log: >>>>>> On 'xml-schema-validation-improvements' branch: >>>>>> >>>>>> Always use installed lxml and rnc2rng Python packages to validate XML >>>>>> output >>>>>> of `svn --xml` in tests. >>>>>> >>>>>> This change also makes these package mandatory for running tests >>>>>> unless new >>>>>> option `--disable-xml-schema-validation` is specified. >>>>>> >>>>> >>>>> [...] >>>>> >>>>> I had a few minutes to spare this afternoon so I checked this commit >>>>> and did three tests on Ubuntu 26.04: >>>>> - With python3-lxml install: make check succceeded >>>>> - Without python3-lxml installed: make check failed with error >>>>> messages, this in the summary: >>>>> [[[ >>>>> There were some XML validation errors, >>>>> checking/home/dsg/svn_branches/xml-schema-validation-improvements/tests.log >>>>> XML: Module lxml.etree not found >>>>> ]]] >>>>> - Without python3-lxml: DISABLE_XML_SCHEMA_VALIDATION=true make >>>>> check succeed. >>>>> >>>>> I think this looks good - fails by default but can be made to work. >>>>> >>>>> I also tried cmake/ctest but that failed spectacularly with (62 tests >>>>> failed) or without lxml (61 tests failed!), so I presume something else is >>>>> wrong. >>>>> >>>>> Only thing remaining is to document this. "make check" is only >>>>> mentioned under "Building from a Tarball". Maybe it deserves a separate >>>>> section ("D. Running the test suite"). Possibly even restructuring "II. >>>>> Installation" to separate our three build systems (autoconf/make, vcproj >>>>> and CMake) and having a separate section on testing under each? Do we do >>>>> this on the branch or after merging back to trunk? >>>>> >>>>> >>>> Thanks for testing this! >>>> >>>> There are a few things that still need to be done: >>>> >>>> 1. I learned that we can actually use the builtin Python XML package to >>>> check the structural validity of XML responses. While this isn't the same >>>> as fully validating the response against a schema definition, it still lets >>>> us catch many cases where we produce invalid XML. >>>> >>>> In r1935590 I implemented this improvement and I'm now thinking about >>>> making this mode the default, enabling full schema validation only if >>>> explicitly requested. This way maintainers that run tests in their >>>> pipelines can still get an adequate level of checking without being >>>> required to install additional packages. And we ourselves will enable the >>>> full validation in GitHub Actions. >>>> >>>> >>>> >>>> I like this approach, with one small nit – there's no need to >>>> explicitly request schema validation, just check at runtime if the required >>>> modules (lxml and rng2rnc) are available and if they are, perform full >>>> validation. There is/was already code in svntest/main.py to check that. >>>> >>>> >>> Since we now have the always-on XML structural check, I'd say we should >>> pick between two options: >>> >>> A) Make full schema validation implicit when the modules are available. >>> B) Have an explicit `--enable-xml-schema-validation` switch to enable it. >>> >>> I'm fine with either of these approaches. However, the explicit >>> `--enable-xml-schema-validation` approach has a couple of advantages: >>> >>> - It makes test results, such as error messages, predictable and >>> independent of the environment. >>> - It gives users a way to check whether their environment is suitable >>> for full XML schema validation. If a user passes >>> `--enable-xml-schema-validation` and the modules aren't present, that's an >>> error rather than a downgrade of the check. >>> >>> That said, I'm also okay with the implicit approach (A) that you >>> described. >>> >> >> I would prefer option (C): Make full schema validation default (failing >> if lxml/rnc2rng is not available) unless explicitly disabled. (I believe >> this is as currently implemented on the branch). >> >> My argument is that we risk forgetting to run the schema validation tests >> if they are not enabled. Better to force developers to install the >> dependencies to be able to run the tests - or use the option to downgrade >> the check. I believe the lxml and rnc2rng dependencies are relatively minor >> but I didn't check anything other than Ubuntu. If they are difficult to >> install on any platform, we can maybe have different default per platform? >> >> I can see how (C) may look appealing. > > But in practice, I don't think the difference between structural > (always-on) validation and full validation is wide enough to justify that > cost. Structural validation already catches most common problems and full > validation will still run regularly through GHA. If someone wants stronger > guarantees for his environment, he can opt in. > > If we want to attract more contributors and maintainers, I'd say we should > make the development environment friendlier rather than more complex. > Options (A) and (B) don't force developers and maintainers to adapt their > workflows and environments or to explicitly disable validation. > > With that said, how strongly do you feel against proceeding with option > (A) or (B)? >
I see your point and it is a balance. I don't think it is a major complexity to ask for two extra Python modules, it is akin to what happens if you don't have libutf8proc-dev installed when you run configure: 'configure: error: Subversion requires UTF8PROC; install it or re-run configure with "--with-utf8proc=internal"'. To summarise my position: both A and B are acceptable although I still prefer option C. However I see emerging consensus for option B and I won't stand in the way of consensus - it is more important to me that we can get this sorted out than to advocate for my pet solution to the problem. Just let's make sure we have full schema validation enabled GHA on each one of [check, davautocheck, svnserveautocheck] (doesn't have to be in every possible combination of OSes and dependency versions). Cheers, Daniel
