On Tue, 30 Jun 2026 at 19:16, Daniel Sahlberg <[email protected]> wrote:
> 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). > > I implemented option (B) and merged branch to trunk in r1935776. One small note: compared to the version we discussed, the branch now uses a simpler `--check-xml-schema` option instead of lengthy `--enable-xml-schema-validation`. Thanks! -- Ivan Zhakov
