Fix regression: duplicate idents in *Specs
#681
Conversation
Refining how the zero-th pass for ODD processing handles gathering up duplicate named specs
|
Fixed a cardinality constraint, so all tests are now passing. Going to mark this as ready for review |
|
Will this also fix #680? Happy to test it if so! |
There was a problem hiding this comment.
I have tested a simple chain (customization A reads output of odd2odds.xsl on customization B), and it failed miserably using dev branch Stylesheets and succeeded using this branch. (And the output RNC is valid and looks reasonable, and the output HTML looks no worse than expected.)
I have also run all tests in Test/ and Test2/ (except the one that requires XeLaTeX), and they pass.
I am mildly trepidatious because I do not actually understand the change made, but still giving a thumbs up because it looks reasonable, and (as stated above) every output I look at is either the same or better.
There was a problem hiding this comment.
@joeytakeda The code changes look sensible to me. Kudos for typing the variable to help clarify this code, and for moving to value (one to one) comparison.
|
This is passing Test 1 for me, and mostly Test2 (except I think something is borked about the " [exec] BUILD FAILED @lb42 Have you had a chance to test this on a chained ODD? I'd like to see how this does on one of your chained scenarios and see how much this simple yet profound fix addresses those other tickets. We're preparing to refridge (or is that heat) this Friday! |
|
I attach a zip containing a compiled ODD and another ODD which uses it. This produces a schema OK, but has trouble running odd-xhtml conversion : both in oxygen and using teitohtml :-( |
|
I spoke too soon -- the RNG generated is also wrong. Advice on how to test/ confirmation that "it works for me" both gratefully received! |
|
The script I run says |
|
@lb42 — |
|
Ah yes, just ignore that include. It's prose waffle not relevant to the odd |
|
So I tried the the same 3 cmds @lb42 describes, first using the Stylesheets dev branch, and then using this branch. (Note though, that because I did not specify --localsource, I am building against the version of p5subset in each Stylesheets repo; I checked, though, they are the same as each other, although probably not the same as the local one Lou used.)
I then compared each output of these cmds when run using the dev branch of Stylesheets to the corresponding output when run using this branch. The HTML files were the same (except for timestamps and comments). The eltec-library.xml files seem vastly different from one another. At least in 1 or two cases, though, the difference is that the dev-generated version has duplicate code that the version generated from this branch does not. However, there are lots of other differences, too. The RELAX NG files, on the other hand, were only somewhat different. But at least for most of the differences which I looked at in detail, it seems to me the version from this branch is the better one.
I am not entirely sure what all this means other than a) ODD chaining is quite fragile, and b) while I still think @joeytakeda’s change, here, is probably an improvement, it seems to me more testing is probably in order. |
|
Thanks for confirming that the recent changes made to oddtoodd have fubarred my nice eltec schema! |
ebeshero
added
the
priority: releaseBlocker
|
@lb42 @sydb In our Council meeting today, we discussed the complexities of testing to try to resolve the chained ODD processing problems, and thanks to @joeytakeda for stepping us through what he tried and precisely what the problems are. We're marking this as a release blocker. We're talking about testing previous Stylesheets versions in addition to this branch to compare what's going on and see when / how we introduced the problem. More soon, we hope! |
|
It seems pretty clear that whatever is going wrong with my ELTeC schema has nothing to do with ODD chaining as such. There are other examples of ODD chaining which don't exhibit this behaviour, and there is a stand alone version of the ELTeC schema (attached as a text file) which does |
|
@lb42 It turns out you're correct, that what was going wrong with ELTeC schema wasn't to do with ODD chaining at all. We investigated the problem in our Stylesheets meeting today and discovered an interesting problem: What generates the duplicate idents is specific this problem:
We tested this with other attributes, too, and we consistently generate the same error that we're now working on in this new ticket: #687 Basically you can address this by updating your reference to |
|
Thanks for the comment @ebeshero, but I don't think that's my problem. The eltec-standalone.txt file which I posted on this ticket does not reference @Calendar at all (the line you quote is commented out), but this file still generates errors when processed to HTML. Please try downloading my test file, renaming it to .xml, and open it with an Oxygen using the most recent framework release (4.8.44) and you will see the problem.(Interestingly, if I use the previous version of the oxygen framework, the problem goes away, which suggests strongly that this is something you guys introduced... |
|
@lb42 No indeed, we found the problem in eltec.xml, where from line 122 we see the following: <classSpec type="atts" ident="att.datable" mode="change">
<attList>
<attDef ident="calendar" mode="delete"/>
<attDef ident="period" mode="delete"/>
</attList>
</classSpec>We found the Relax NG generated from this ODD as well as the HTML to be bearing multiple definitions of the very You're right that this is based on something we changed. That change was simply moving |
|
Hi @lb42 — just for completeness' sake, I ran I believe your issue here (as it was in #645 and #678) was duplicate elementSpecs with the same ident (in this case, it was the elementSpec for the TEI element, defined around lines 147, 319, and 888). This was a regression in the Stylesheets (caused by change to ensure uniqueness in element names across namespaces), but since none of our test files had duplicate elementSpecs, this was never caught. At the very least, this will make it into the new release (which should occur within the next few weeks). As far as I understand it, though, the oXygen framework will only update with a new oXygen release (though @martindholmes will know more about that) |
|
Sorry I was insufficiently clear. "the problem" i am referring to is nothing to do with the @Calendar related bug (thanks for fixing which). The file "eltec-standalone" doesn't use eltec.xml -- as the name suggests it is a standalone unchained version. But it has something wrong with it because it fails to generate HTML. The message generated is |
|
Ah, I have now seen Joey's comment. Thanks! There is indeed a duplicate declaration for TEI in the standalone file (there are three in fact) and removing it removes the problem. BUT this is very worrying: the ODD system is intended to permit multiple declarations for the same object. A processor has to know how to reconcile/unify them sensibly... |
|
@lb42 I don't believe there is a canonically sensible way to handle duplicate definitions, and the ODD spec doesn't provide such an algorithm, so I think ATOP will treat them as errors and argue that they should be invalid. |
|
@martindholmes It rather depends what you mean by duplicate definition. If i supply an elementspec containing just an exemplum for example is that a duplicate? |
|
Yes. You can devise scenarios in which a human could effortlessly combine the instances into a coherent whole, but it's difficult to write an algorithm to determine when that pertains and when it doesn't. And if you can easily combine them, then why not do it in your ODD? |
|
@martindholmes Seems to me that if i say |
|
@lb42 As far as we know (based on all the ODDs we collected as part of our initial survey when starting ATOP), you're the only person who uses duplicate elementSpecs -- there was one other case but it was a genuine error, with the same elementSpec appearing twice. I think if you have a very clear idea of exactly how an ODD processor should resolve all the potential permutations arising from a multiplicity of elementSpecs with the same |
|
I am happy to withdraw the provocative term "lazy"! I don't think you're really addressing my point though -- which is that all ODD writers, including ones not yet born, need clear guidance on how far they need to go in combining into a single spec all its constituent parts. |
|
I have to second @martindholmes here after we've repeeatedly struggled with repairs and tests for odd2odd.xsl. Trying to figure out which conditions are being handled and when is a pretty complicated process in the current odd2odd, and trying to account for them in ATOP won't be easy. Anyway, @lb42 's ELTeC files did prove immensely helpful for finding that serious problem with processing re-located attribute classes! But having to reconcile multiple elementSpecs sharing the same |
|
@ebeshero The way I conceive of chaining is that it's a sequence of discrete steps, so no matter how long the chain, the processor should only ever have to resolve one unique spec in the customization against one unique spec in the preceding source ODD. |
Attempting to resolve the problem of duplicate idents in specs (as mentioned in #678 and #645 and potentially others). At the time of my filing this PR, Test1 is failing (in particular, test21), so more investigation (and more testing) is required before this can be merged.