Oddity in e²->e³ conversion

[DiapDealer]

Originally Posted by KevinH

Really not sure how the opf is read (ie by recreating it from parsed form; or by sneaking around the wrapper to directly access the opf file) but epubcheck and epubveri plugins could have made different choices here.

According to the plugin's thread, there have been some changes to epubveri in that regard. Looking at the code, I can see that epubveri doesn't have an option to validate an unzipped epub, so a temporary epub is created. In earlier versions, the copied opf was being overwritten using get_opf (which I believe is Sigil's parsed rebuild of the opf). That was changed to using readotherfile. This was in an attempt to verify unsaved edits. But it was resulting in line numbers that didn't match what the user was seeing in Sigil when reporting errors.

https://www.mobileread.com/forums/sh...7&postcount=20


I may be putting too much emphasis on the silent opf rebuild, since it's been that way for a long, long time. Any save is going to correct a missing closing tag in the opf afterall. It just seemed odd to me that a problematic epub could be opened, validated, and closed (with the pre-existing error still intact) with no warning messages whatsoever

[KevinH]

It should not happen as SaveAllToDisk is used in prep for any plugin and Save by Sigil. I can not see how using bk.readotherfile in a plugin gets something different. Even if it is just missing the xml header line if SaveAllToDisk actually is working properly. So I am with you on this. Something is strange. I will explore this with some debug output in ResourceObjects/TextResource.cpp to make sure its SaveToDisk routine is properly being hit (for the opf's bookpath). The OPFResource inherits from XMLResource which in turn inherits from TextResource, so that is the routine that should be triggered unless we override it in OPFResource itself.

[KevinH]

Actually, OPFResource overrides SaveToDisk so I will take a look at it.

[DiapDealer]

For what it's worth: The latest version of the epubveri is now returning the same two errors that epubcheck did in the original post. The missing closing spine tag is not being reported by epubcheck or epubveri. So that leads me to believe that plugins are getting the same opf (corrected to add the closing spine tag) that Sigil is seeing. The corrected closing tag has not been saved to the epub yet, but both validators are ignoring it. Which leads me to believe that the opf has indeed been saved to disk (to Sigil's scratch folder) before being handed off to the plugin.

[KevinH]

Whew! That is good to hear!

Staring at tiny code in my phone was killing me!

[DiapDealer]

I'm still a little concerned about how an epub with a bad opf can potentially be opened, validated with no errors, and then closed (without saving) with an opf error still intact. With no warnings.

But as I said, I think that particular situation has been a possibility for a long time. But the epub would usually be saved at some point, or something else would be flushed to disk to change the modified status of the epub.

And if the Epub3-itizer plugin gets its fix, we should be in a good place.

[Kayadelenium]

Originally Posted by DiapDealer

why the epubveri plugin can see the missing spine closing tag while Sigil and epubcheck cannot

Author of epubveri here. It is not a parsing difference, and I would rather kill that idea than let it stand. I do not have DNSB's book, so I built the smallest one with the same fault — an OPF whose </spine> is simply absent — and gave it to epubcheck 5.3.0:

FATAL(RSC-016): content.opf(15,3): The element type "spine" must be terminated by the matching end-tag "</spine>".

The difference is which bytes each tool was given. The plugin asks Sigil for the container with copy_book_contents_to, and for the OPF that routes through readotherfile, which has this in wrapper.py:

if id == self.opfbookpath:
    if id in self.modified:
        return self.build_opf()

So Sigil hands back its live rebuild only when the OPF is marked modified, and the file from the ebook root otherwise. In this case it was the second — which is how I know, without having seen the book, that the OPF was not marked modified after the conversion plugin wrote it. The plugin validated what was actually in the container, and the container was malformed. Saving fixed it because saving is what flushes the repair, exactly as KevinH describes.

So epubveri was not cleverer; it was looking at the file rather than at the model. Which of the two epubcheck was given in DNSB's run I cannot tell from here — run over the container it reports this fault, as above, so if it stayed quiet it was reading something else.

Now the part I would like your opinion on, because this thread has found a hole in my reasoning rather than in my code.

Reading the file instead of the model was a deliberate choice, and I made it for one reason: line numbers. Version 0.1.0 substituted get_opf() unconditionally, and build_opf re-serialises from Sigil's model — it sorts the manifest by id and rewrites each item — so the plugin was reporting line numbers into a document nobody could see. On one book a manifest entry sat at line 91 in the file, 95 in the rebuild and 96 in what Code View displayed. Taking the file from the ebook root put the numbers back on the text in front of the user.

DNSB's book breaks that reasoning. Here Code View shows a </spine> that is not on disk, so the file and the display disagree, which is the one thing my choice was supposed to guarantee. The line numbers are then right about the file and wrong about the screen, by one line, from the spine onwards.

So, two questions for the three of you, since you know Sigil far better than I do:

1. Which artefact should a validator inside Sigil report on? The container as it stands on disk is what ships if the user never saves — and it is what caught this — but it is not what they are looking at. The model is what they see and what a save will write, but it will never show them a fault Sigil has already quietly repaired. I currently do the first, and after this thread I am no longer sure that is right for an editor plugin as opposed to a command line.

2. Is there any way for a plugin to know that Sigil repaired something while loading? If there is, the honest thing is to say so in the results — "this was repaired in memory, save to write it" — rather than to report a fault the user cannot find in Code View, or to hide one that is really there. Comparing the file against build_opf() is not it: the re-serialisation differs from the original even when nothing is wrong.

Either way, thank you for chasing this one down to the line in opf_newparser.py. That is a better outcome than the validator noise that started it.

[KevinH]

To clarify, Validators inherit from the output container so they should be using only the copy_book_contents and that in turn uses readotherfile but since a validator plugin and not the edit plugin, the opf should never be marked as modified by the plugin. So you should get the opf file itself not, the rebuild. Rebuild is used by edit plugins that can make changes to its copies and therefore the only valid opf if modified is the rebuilt one.

So in a validator plugin if the save all to disk is indeed working as expected for the opf it should result in any Sigil internal user made modifications or fixes to the opf being flushed to disk first before any plugin is launched and it should result in readotherfile returning a file that exactly matches what Sigil is showing and using internally. If that is not happening, it is a bug that I will track down and fix upon my return next week.

As for changes to notify for opf fixes, they are not needed by design as Sigil requires a 100% machine readable opf at all times and so auto repairs common xml issues with each read, to prevent Sigil from losing data. That is not changing.

So somewhere the built-in flush to disk the happens before any plugin is not happening as expected as it seems to forget to flush auto-repairs for some reason that I will look into.

Hope this clarifies.

[Kayadelenium]

Originally Posted by KevinH

the built-in flush to disk that happens before any plugin is not happening as expected

Thank you — that answers both questions, and the first one settles a choice I had made for a reason that turns out to be shakier than I thought. Since you are away, here is what I could work out from this end, in case any of it saves you time when you get back. All of it is from your published source — line numbers are from master today, so they may be a little off against a release — and none of it is a diagnosis; I have marked the one place I am guessing.

1. How the plugin sees the file at all — it has no other option

Worth stating plainly, because it means no plugin can be doing anything clever here. wrapper.py:213 initialises self.modified to an empty OrderedDict, and every later entry comes from the running plugin's own calls. Sigil does not seed it. So the readotherfile special case can only return build_opf() for an OPF that this plugin has already changed, and a validation plugin — which changes nothing — always receives the file from ebook_root. That is not a design decision of mine; there is no other outcome available.

2. The flush looks right at every step I could follow

I went looking for a place where the OPF might be skipped and could not find one:

• PluginRunner.cpp:283-286 — SaveTabData() then m_book->SaveAllResourcesToDisk(), under your own comment about flushing before the plugin. Called.
• Book.cpp:1446 maps SaveOneResourceToDisk over m_Resources.values(), and FolderKeeper.cpp:448 puts m_OPF into m_Resources. The OPF is in the list.
• Resource.h:230 declares SaveToDisk virtual and OPFResource.h:86 overrides it, so the dispatch reaches OPFResource::SaveToDisk rather than the base.
• OPFResource.cpp:342-346 runs CleanSource::ProcessXML — XMLPrettyPrintBS4 — then SetText, then TextResource::SaveToDisk. The early return at TextResource.cpp:97 (!m_CacheInUse && !m_IsLoaded) cannot fire on this path, because SetText has just set m_CacheInUse at TextResource.cpp:77.

So on that reading every plugin launch should rewrite the OPF, pretty-printed through BS4 — which would also have closed the spine tag on its way past.

A cheap test of that, using what is already in this thread: DNSB attached the converted OPF in post 8. If it is neither pretty-printed in Sigil's style nor closed, then no OPFResource::SaveToDisk ever wrote it, and the question is why the call did not reach there rather than what it did when it arrived. That is a two-minute check and it would split the search in half.

The one place I would instrument, and here I am guessing: SaveAllResourcesToDisk uses QtConcurrent::blockingMap, so SaveOneResourceToDisk runs off the GUI thread, and TextResource::SetText carries your own comment about delaying the QTextDocument update when it is not called from the main thread. GetText() on that path reads m_TextDocument->toText(). Printing what GetText() returns for the OPF at that moment would say whether the text being processed is the one you expect.

3. The other door, which is the part I would fix regardless

Separately from the flush: launcher.py:157-158 calls container._w.write_opf() at the end of an edit plugin, and wrapper.py:413-422 writes build_opf() straight to disk as bytes — no parse, no mend, no check. Since writeotherfile refuses the OPF as a protected file (wrapper.py:929), that write-back is the only way a plugin's OPF text reaches the book, and nothing on it asks whether the text is well-formed.

You have fixed Epub3-itizer, so this particular book will not happen again. But the next plugin that builds an OPF by hand walks through the same door, and the fault lands silently in the user's book rather than in the plugin author's lap. A single ElementTree parse there — refuse the write and report it — would have turned DNSB's afternoon into an error message at the moment the bad text was produced. Happy to open a PR if that is welcome, or to leave it entirely alone if you would rather decide the shape yourself.

4. A detector, if it is useful

Fifteen lines, no dependencies, works on any book. Drop it in as a validation plugin and it answers "did the flush happen" in one click:

import os
from xml.etree import ElementTree as ET


def run(bk):
    bookpath = bk.get_opfbookpath()
    raw = bk.readotherfile(bookpath)
    if isinstance(raw, bytes):
        raw = raw.decode("utf-8", "replace")
    try:
        ET.fromstring(raw)
    except ET.ParseError as failure:
        bk.add_result("error", bookpath, "-1",
                      "the OPF a plugin receives is not well-formed: %s"
                      % failure)
        return 0
    bk.add_result("info", bookpath, "-1",
                  "the OPF a plugin receives is well-formed (%d bytes)"
                  % len(raw))
    return 0

I ran it both ways rather than writing it from memory: against a sound book it reports well-formed, and against a book whose OPF I stripped the closing spine tag from on disk it reports mismatched tag: line 15, column 2. Worth knowing from the same run: the launcher's own Opf_Parser reads that malformed OPF without complaint and the plugin starts normally, which is presumably why nothing upstream of the plugin noticed either.

One thing that made all of this cheap, in case it is useful to anyone else here: launcher.py can be driven directly, with no GUI — hand it a book directory, an output directory, "validation" and a plugin path, run it under Sigil's bundled Python, and you get the real Wrapper and the real result XML. I had been testing my plugin against a container of my own making because I had written down that Sigil could not be scripted. The window cannot; the launcher can, and every Sigil behaviour my plugin got wrong so far had been learned by installing a zip and clicking.

Not chasing this — it is your bug to fix in your own time, and none of it is urgent from my side. It just seemed better to hand over what I had found than to sit on it.

[KevinH]

I am well familiar with the code as I designed it close to 10 years ago. And I have been programming for over 50 years and do understand how to debug. Note: ePub3-itizer is an *output* plugin not an edit plugin so it is the Sigil Importer for epub which allowed it in with a bad form, which it was designed to do to handle poor epubs. And PluginRunner does do an OPF Well-formed check before accepting the results of an edit plugin and does so for every xml and xhtml file so that is not the problem.

Either the flush to disk is not working in some way (no rtti so not clear if more explicit qobject cast is sometimes needed or that somehow a broken opf was loaded by Sigil's ImportEpub and no further call to load the opf for any Sigil tool was ever made meaning no auto repair was done. Either way we will take care of it in a few days when I am back and have access to more than my phone. Thank you but your help is not actually helping in this case. The problem is most likely in the Sigil ImportEpub code that auto repairs before any initial read but purposely does not update the opf immediately after unpacking it to prevent comments in the opf from being initially lost. Normally this does not hurt anything as whenever the opf must be read by Sigil it is auto-repaired first then saved upon a flush. So this is an odd case as no further reading of the opf seems to have been done.

We will handle it from here.

[Kayadelenium]

Originally Posted by KevinH

The problem is most likely in the Sigil ImportEpub code that auto repairs before any initial read but purposely does not update the opf immediately after unpacking it.

Thank you — that is upstream of anywhere I got to, and it fits what we saw.

I will leave it with you and watch the thread. Happy to test anything on my side once you have had a look.

[KevinH]

Okay, I now have some time to look at this. The bug that created the missing closing spine tag existed in ePub3-itizer. It has been identified and fixed by BeckyEbook, and will be released once I have some time. But since ePub3-itizer is an output plugin, it generated a standalone epub3 that was broken.

If you ran epubcheck or epubveri on that standalone epub3 (not using a Sigil plugin) they should both detect and report that broken epub3.

Now, if you instead load that broken epub3 into Sigil, Sigil's Importer is built to try to load even a broken epub by auto repairing the opf during the load. Being able to load a broken epub is a critical skill in any epub editor.

I did check and during Sigil's ImportEPub, the opf file is auto repaired if it can be fixed, and that fixed version of the opf is written to the Sigil OPFResource itself and save to disk is called, so that what is loaded inside Sigil is correct. If the auto repair can not fix things, an error is thrown in Sigil during import and the user is warned and in some cases no load at all can be done depending on the severity of the error.

Now comes the interesting part ... what is loaded inside Sigil should be fixed and saved to Sigil workspace to disk. If you open the OPF tab for editing, this should show as fixed.

But until a file save has been done in Sigil, the original broken epub3 has not been modified.

So if both epubcheck and epubveri plugins are run as validation plugins they should both see the exact same thing as shown in the OPF tab. That assumes they are using the validation container copy_book_contents which takes the unpacked files inside Sigil and creates a standalone unpacked copy which can then be zipped up using Sigil plugin apis.

But there are ways to go around the plugin wrapper.py and access the file path of the original broken epub3 that was loaded into Sigil. If that was done by either plugin, that is a bug in that plugin's code as it is not then validating what is inside Sigil (ie is missing any unsaved user and Sigil changes).

I intend to fully validate this entire sequence in the debugger. Once validated, I will start to examine how Doitsu's epubcheck and epubveri actually create the epub zip file and where it gets things from.

So this is just a head's up for what I will be doing to try to track down what on earth is going on.

[KevinH]

So the reason why Doitsu's epubveri comes up with something different from epubcheck is that as a short cut the epubveri script is using the the following code snippet:

#=====================================
# main plugin routine
#=====================================

def run(bk):

    # path to the curent epub or an empty string if the book is new
    epub_path = bk.get_epub_filepath()
    
    # check if the current epub was modifed or the current book is brand-new
    if bk.get_epub_is_modified() or epub_path == '':
        print('Creating temporary epub...')
        # remove temp epub, in case the plugin previously failed
        temp_epub_path = os.path.join(bk._w.plugin_dir, bk._w.plugin_name, 'temp.epub')
        if os.path.exists(temp_epub_path):
            os.remove(temp_epub_path)
        
        # create a temporary epub
        copy_current_epub_to_temp_epub(bk)
        if os.path.exists(temp_epub_path):
            epub_path = temp_epub_path
        else:
            print('Failed to create temporary epub:', temp_epub_path)
            return -1

as it gets the path to the epub that was loaded into Sigil, and then checks to see if it was modified inside of Sigil and it seems to think that that the epub must be identical to what is in Sigil when Sigil may have run Mend On Open, added doctypes, or had its opf repaired when it was loaded. The Sigil Modification flag is only set when the user makes changes to the epub, not when Sigil auto repairs the opf or run Mend on Open).

A true validation plugin should use the approach of zipping up the internal Sigil files to create a zip (as is done if modified is true or if opening a new epub not from a path) and not use the epub that has been loaded into Sigil. So until the first user save, any Mend on Open fixes or OPF repairs, will not be reflected into the origial epub.

That is why epub_tools code was created to make creating the epub zip archive easy.


So we have a choice to make.

A. we can set modified to be true on all first loads where a repair was done either to xtml or opf files. That should be doable.

or

B. we simply change the epubveri plugin validation code to remove that act of bypassing things and have it always validate what is inside Sigil


I think "B" is the more correct option. But "A" maybe should also be done.

But that code snippet does explain the difference between epubcheck and epubveri results.

I think it is more correctly done in epubcheck plugin with this code that unconditionally creates a temporary zip epub archive always using the Sigil's current files

    #-------------------------------------------------------------
    # copy current epub to temp epub
    #-------------------------------------------------------------
    with make_temp_directory() as temp_dir:

        # copy book files
        bk.copy_book_contents_to(temp_dir)

        # create mimetype file
        with open(os.path.join(temp_dir, "mimetype"), "w") as mimetype:
            mimetype.write("application/epub+zip")

        # zip up the epub folder and save the epub in the plugin folder
        epub_path = os.path.join(bk._w.plugin_dir, bk._w.plugin_name, 'temp.epub')
        if os.path.isfile(epub_path):
            os.remove(str(epub_path))
        epub_zip_up_book_contents(temp_dir, epub_path)

So no critical Sigil bugs here, just a difference in how the two plugins get access to the files inside Sigil versus using the original epub zip file path.

[KevinH]

After thinking about this, I will make the "A" modification to Sigil source and leave the validation plugins to decide for themselves if they want to change things so that epubveri uses epubcheck's approach to always test the files that are actually inside Sigil.

[KevinH]

Update, it seems that ImportEpub *always* XMLPrettyPrints the OPF using our internal bs4 variant and lxml because they naturally strip out all whitespace from pure xml during parsing. So even without a well formed error, the chances of the line numbers matching the original opf are next to none.

Again parsing of the opf must be robust to errors for any epub editor to work as the opf must always be kept in a machine readable state and be well formed. Loading a broken OPF is just not a good idea ever.

Sigil has used this approach safely for thousands upon thousands of users for over 15 years so I am loathe to change it for one plugin that tries to save a step and not work with Sigil's internal files.

So at best Sigil can simply set the modified flag to true on any initial load of any epub because the OPF whitespace will most probably have been changed at the very least.

Therefore, it would be better if validation plugins did not try to work around the Sigil contents to try to read the epub that was initially loaded and instead always only worked on the versions of the files provided by Sigil, just like Doitsu's epubcheck plugin has worked for years.

Hope this helps explain my reasoning.