bugGNU Octave - Bugs: bug #59273, [octave forge] (io) can't reliably...

 
 

bug #59273: [octave forge] (io) can't reliably load XLSX files in release 2.6.2

Submitter:  Sébastien Villemot <svillemot>
Submitted:  Thu 15 Oct 2020 09:37:52 AM UTC
   
 
Category:  Octave Package Severity:  3 - Normal
Priority:  5 - Normal Item Group:  None
Status:  Fixed Assigned to:  philipnienhuis
Originator Name:  Open/Closed:  * Closed
Release:  * other Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sat 24 Oct 2020 06:53:57 PM UTC, comment #20: 

Closing bug report after some more verification that the fix works.

Philip Nienhuis <philipnienhuis>
Group Member
Thu 22 Oct 2020 04:35:31 PM UTC, comment #19: 

Sébastien,

Before fixing the Debian package please remove the debug statements in xlsopen:

  • add a semicolon at end of L.306 (getinterfaces() call)
  • remove printf statement at L.316


and in __OCT_spsh_open__.m remove the commented out code.

You can also get the files from the io repo, I've already pushed them. xlsopen.m differs a bit (no echo of interfaces beyond L.333) but that functionality isn't in io-2.6.2 anyway.

Thanks for your cooperation in fixing the issues.

An io-2.6.3 will hopefully be released soon after bug #59277 and some other code refinements.
Are you interested in testing that before release, or would you prefer to wait until the release is made?

Philip Nienhuis <philipnienhuis>
Group Member
Thu 22 Oct 2020 10:47:33 AM UTC, comment #18: 


commentaire #17 :

> Please try with attached _OCT_spsh_open_.m and _ods_get_sheet_dims_.m, bth in private/ subdir.
> As usual, on top of the other latest file versions in this bug report :-)


Thanks, it now works, all the tests pass. I am going to fix the Debian package using those patches.

> I'm a bit surprised you got this error. This code apparently hasn't changed since > 7 years; I can only surmise it is due to the test.ods spreadsheet in the test directory. That spreadsheet is kosher, that is, it gives no errors when reading with LibreOffice and any of the other spreadsheet interfaces, so the bug must be real.
> Which makes me wonder if the debian tests have changed the last year, IOW, why hasn't this been catched before.
> But then again, maybe maybe something changed in Octave-6+ that provoked the bug. File encoding handling?


As said before, the problem with ODS/OCT started to appear when I applied the patches that you added in comment #10.

The Debian testsuite has not changed for years. Also note that I use Octave 5.2.0 for all my tests, not Octave 6+. And I just verified that io 2.6.1, using the same tests and the same environment, passes all the tests.

I have no idea why your recent changes triggered the bug in ODS/OCT. But there is clearly a causality relationship.

As far as I’m concerned, this bug is now fixed. Thanks for your help.

Sébastien Villemot <svillemot>
Wed 21 Oct 2020 09:00:55 PM UTC, comment #17: 

Please try with attached _OCT_spsh_open_.m and _ods_get_sheet_dims_.m, bth in private/ subdir.
As usual, on top of the other latest file versions in this bug report :-)

I'm a bit surprised you got this error. This code apparently hasn't changed since > 7 years; I can only surmise it is due to the test.ods spreadsheet in the test directory. That spreadsheet is kosher, that is, it gives no errors when reading with LibreOffice and any of the other spreadsheet interfaces, so the bug must be real.
Which makes me wonder if the debian tests have changed the last year, IOW, why hasn't this been catched before.
But then again, maybe maybe something changed in Octave-6+ that provoked the bug. File encoding handling?

That said, the entire OCT interface comprises fairly fragile code. Regexps, string searches for XML nodes and tags, it isn't all very robust but apparently just enough.



(file #50042, file #50043)

Philip Nienhuis <philipnienhuis>
Group Member
Wed 21 Oct 2020 04:19:38 PM UTC, comment #16: 

.ods in OCT error?
Remarkable, that part hasn't been touched at all for a long time.
But the cause may be elsewhere in the whole spreadsheet functions fandango, I'll have a look.

FTR, sorry that this is such a long-winded bug report.
At my side it's a bit hard to mimic debian's QC, so I am glad you catched this.

As I wrote in bug #59277 it started with a programming error by me already for io-2.6.1 that silently sneaked through even Debian's checks. Yet the POI bug here and bug #59277 finally helped uncovering it.

All in all this gave rise to some more polishing of various support scripts which I'll do the coming days.
I hope to be able to release io-2.6.3 before Octave-6.1 gets released.



Philip Nienhuis <philipnienhuis>
Group Member
Wed 21 Oct 2020 12:12:44 PM UTC, comment #15: 


commentaire #14 :

> Sure I saw those files, you mean "testhelper.m"?


Yes, and also the various individual files in the same directory that have the same name as the tests: xls-default, xls-poi, …
Each of these files contains information about the test, the most important being the command-line to run the test, which in the present case is a call to octave-cli with appropriate command-line parameters. In particular, all those various tests are executed in independent octave processes.

> Please try again with attached

__POI_spsh_open__.m
> (in private/ subdir) on top of the latest xlsopen.m and private/getinterfaces.m from this bug report.

With this additional patch, all XLS and XLSX tests now pass!

However, there is still a problem with ODS files using the OCT interface, that was introduced in the previous round of patches. I attach the corresponding build log.

Thanks

(file #50034)

Sébastien Villemot <svillemot>
Tue 20 Oct 2020 06:29:12 PM UTC, comment #14: 

Sure I saw those files, you mean "testhelper.m"?

Anyway, it may be a bit easier.

I just tried with various POI versions and it seems indeed the if block check for POI versions in __POI_spsh_open__.m may be unneeded.
Which makes me wonder where I got those call versions from, I suppose Javadocs but I can't find the actual page now. But I also remember that I didn't introduce that if block for nothing.
It's this cset:
http://hg.code.sf.net/p/octave/io/rev/f55bdd2e264b
FTR I tried with POI-4.1.2 & 4.0.0 & 3.1.7 & 3.1.5.

Please try again with attached

__POI_spsh_open__.m
(in private/ subdir) on top of the latest xlsopen.m and private/getinterfaces.m from this bug report.

There also seems to be an issue that if the first .xlsx interface doen't work ("crashes/errors out") ensuing versions won't work either.However in my own testscript (test_spsh) I didn't see that happen.


(file #50033)

Philip Nienhuis <philipnienhuis>
Group Member
Tue 20 Oct 2020 04:44:39 PM UTC, comment #13: 

As explained in comment #7, you can see the commands called from the testsuite there: https://sources.debian.org/src/octave-io/2.6.1-3/debian/tests/

However, it looks like we’ll need a lot of back-and-forth before we can solve the problem in this way, since you cannot replicate it on your machine(s).

If you think it can help, I can give you access to a Debian container where the problem is reproducible. If you want to go this way, please contact me by email (my address is visible in my Savannah profile).

Sébastien Villemot <svillemot>
Tue 20 Oct 2020 04:37:06 PM UTC, comment #12: 

Thanks for the re-trials.

TBH it's unclear to me what is happening in the debian test suite. Apparently a new pristine test environment gets built and then some test are run for which I can't infer how the functions in the io package are invoked (no command lines with input parameters).
I do see the detected interfaces and they make sense to me, give the javaclasspath you gave in comment #4.

What is lacking in the logs is the output of line like eg., L. 341-343 in xlopen:

    if (verbose)
      printf ("Invoking POI ...\n");
    endif


In the example you gave in comment #0:

octave:3> cd ~/debian/octave-io/debian/tests/
octave:4> xlsfinfo('test.xlsx', [], true)


'verbose' = true so I expected some output. But I suppose that the tests suite doesn't specify it.
Maybe variable 'verbose' has to be set explicitly to true (or 1) somewhere at the top of xlsopen.

Other than that I'm at loss what is happening here.
It all works fine on my boxes (Windows 10 and Mageia 7). Those may not be representative and as you wrote io-3.6.1 did work fine (but I believe for the wrong reason - I do suspect that maybe behind your back OCT got involved rather than 'POI & OOXML').

(just a hunch) Does the JVM have enough memory allocated? maybe POI (needing a lot of resources) chokes already at file open time.

(Another hunch) POI has been messing with the WorkbookFactory classes; maybe in private/__POI_spsh_open__.m the call to ..workbookFactory0

        wb = javaMethod ("create", ...
                         "org.apache.poi.ss.usermodel.WorkbookFactory0",...
                         xlsin);


should have a trailing 1 rather than 0, or maybe the version comparison works out incorrectly and the if block is in error (and only the upper stanza needs to remain).

Perhaps you can try with older POI versions ( 4.0).

I'll try to search further. I have little time this week though ...

Philip Nienhuis <philipnienhuis>
Group Member
Tue 20 Oct 2020 01:57:47 PM UTC, comment #11: 

First note that I made a mistake in my previous comment. Your first set of patches (in comment #5) had fixed the loading of XLSX, while the loading of XLS was still broken (and not the reverse as I had initially said).

Now, regarding your new set of patches (in comment #10), they fix some backends while they break new ones. Here is the test summary from our Debian test infrastructure:

xls-default          FAIL non-zero exit status 1
xls-jxl              PASS
xls-poi              FAIL non-zero exit status 1
xlsx-default         FAIL non-zero exit status 1
xlsx-oct             PASS
xlsx-poi             FAIL non-zero exit status 1
ods-default          PASS
ods-jod              PASS
ods-oct              FAIL non-zero exit status 1
gnumeric-default     PASS


(the "-default" suffix means that no specific interface is requested for the given file format).

I attach the full build log. You will in particular see the debug output that are interested in, for each particular combination of file format and interface.


(file #50028)

Sébastien Villemot <svillemot>
Sat 17 Oct 2020 08:13:46 PM UTC, comment #10: 

Sebastien,

Attached are two new versions of xlsopen.m and private/getinterface.m, could you please try these?
xlsopen.m echoes some debug info to the screen, notably about inferred file type and the supported interfaces; I'd be interested in that.

I can't reproduce nor figure out why your test code would work in io-2.6.1 but not in 2.6.2. I tracked back the logs of the io package but AFAICS both files haven't seen relevant changes between 2.6.1 and 2.6.2. The only difference I can think of is that for io-2.6.1, program flow in xlsopen.m fell trough to using OCT interface behind your back, but even that is mere speculation.
A candidate patch could be:
http://hg.code.sf.net/p/octave/io/rev/91353b9092d8
before which the OCT interface was always active even if the user didn't select it.

Also enigmatic is why .xlsx didn't work with any interface but .xls did with the patched xlsopen.m you tried before.
One can try interfaces by hand using the io_testscript function:
io_testscript ("<interface>", "io-test.xlsx")

There was a bug when xlsopen.m was called for the first time, requested interfaces were wiped, which might explain the erratic behavior I saw. But your testsuite doesn't specify any interface argument so this looks irrelevant.


(file #50003, file #50004)

Philip Nienhuis <philipnienhuis>
Group Member
Fri 16 Oct 2020 09:13:55 PM UTC, comment #9: 

ATM most of this bug has been sorted out. Over in bug #59277 a related issue was reported.

There are actually 2 interfering issues:

  1. User selection of an interface is ignored the first time a spreadsheet is opened after loading the io package;
  2. Detection of POI OOXML support can be incomplete;

and a related, more subdued issue:

  • Fiddling with interface support SW by a user (adding/removing jars to the javaclasspath, unloading windows package) while the io package is loaded might not always be adequately detected, let alone catched.

One may wonder if the latter should be detected & catched in the first place (isn't that user responsibility?) but I did implement it to cut down on bug reports (and bad Octave reputation). But it's usually hard to make such code sufficiently robust, let alone avoid performance degradation.

All of these three issues have to be considered for a fix.

Hopefully tomorrow I'll come up with replacement functions that I hope you can try for me.

Philip Nienhuis <philipnienhuis>
Group Member
Thu 15 Oct 2020 06:22:23 PM UTC, comment #8: 

Thanks, so the Debian testsuite does about the same as test-spsh.m - (for those unfamiliar with io package innards:) the latter finds supported interfaces and tests them one by one.

Anyway, see also bug #59277.

(To be continued tomorrow night)

Philip Nienhuis <philipnienhuis>
Group Member
Thu 15 Oct 2020 05:28:45 PM UTC, comment #7: 

In Debian we have a testsuite that checks all interfaces (by enforcing them at test time), and all interfaces used to work well in 2.6.1, see:
https://ci.debian.net/data/autopkgtest/testing/amd64/o/octave-io/7420457/log.gz

This testsuite is run with the Java libraries installed for JXL and POI+OOXML.
The source of the testsuite can be seen at:
https://sources.debian.org/src/octave-io/2.6.1-3/debian/tests/

Those very same tests that used to work with 2.6.1 now fail with 2.6.2 (without your patch). All tested XLS and XLSX interfaces fail (XLS/JXL, XLS/POI, XLSX/OCT, XLSX/POI). The environment has not changed, in particular the same Java libraries are present.

I’ve applied your patches and they fix the testsuite for XLS loading (i.e. XLS/JXL and XLS/POI now work). However it still fails for XLSX/OCT and XLSX/POI.

So you clearly fixed an issue, but there is still something going on.


Sébastien Villemot <svillemot>
Thu 15 Oct 2020 05:26:44 PM UTC, comment #6: 

(Our posts crossed)

Thanks for the javaclasspath echo, that looks good enough. I'm at POI-4.0.0; FTR it turns out that some POI classes of versions 4.0 and 3.15 can be mixed w/o loss of functionality of the io package:

   DYNAMIC JAVA PATH

      C:\Users\philip\Java\poi-ooxml-schemas-4.0.0.jar
      C:\Users\philip\Java\jOpenDocument-1.4rc2.jar
      C:\Users\philip\Java\xml-apis.jar
      C:\Users\philip\Java\xercesImpl-2.11.0.jar
      C:\Users\philip\Java\gwt-servlet-deps.jar
      C:\Users\philip\Java\jxl.jar
      C:\Users\philip\Java\backup\commons-compress-1.18.jar
      C:\Users\philip\Java\commons-collections4-4.2.jar
      C:\Users\philip\Java\dom4j-1.6.1.jar
      C:\Users\philip\Java\xmlbeans-3.0.1.jar
      C:\Users\philip\Java\backup\poi-ooxml-3.15.jar
      C:\Users\philip\Java\backup\poi-3.15.jar


I'd be curious how your original example works in io-2.6.2 with the patched xlsopen/getinterfaces functions of comment #5 when chk_spreadsheet_support.m's output equals 74.

No doubt you know io package's innards :-)
Thing is, I cannot reliably reproduce your issues here, that's why I'm asking you for several trials that perhaps look quite useless for you.

Philip Nienhuis <philipnienhuis>
Group Member
Thu 15 Oct 2020 05:10:50 PM UTC, comment #5: 

Looks like you've hit a bug that has been looming for quite some time.

It boils down to always trying to invoke POI for .xls and .xlsx even if only partial POI support (just for .xls) was present. Turns out I fixed this already in 2017 but after last year's interfaces reshuffling for io-2.6.x that fix got lost.

The other, subordinate issue is L.131 in xlopen.m,
      reqinterface = [];
which needs unconditionally changing to
      reqinterface = "";

... and maybe it shouldn't be there in that form at all.

As a first step, would you please try again with attached xlsopen.m and getinterfaces.m ? (the latter to be put in private/ subdir)

Next up = finding out what else you hit when you did have full POI support. If still needed.

Taking a step backward for some overview it would be better to rethink the "verbose" option. You say that it worked in io-2.6.1 (and before?) but you -unknowingly- may have actually invoked OCT rather than POI&OOXML. xlsopen.m as it stands doesn't give you any clue.


(file #49985, file #49986)

Philip Nienhuis <philipnienhuis>
Group Member
Thu 15 Oct 2020 04:04:42 PM UTC, comment #4: 


commentaire #3 :

> chk_spreadsheet_support.m's return value of 78 is what you'd get with JXL (8), full POI (2 + 4) and JOD (64) support (+ OCT) and 74 is the same with basic POI support(just .xls).
> 4 is the added value for POI-OOXML; see "help chk_spreadsheet_support".


Note that I’m well familiar with all that stuff (I’ve been packaging io for Debian since 2012).

> Again, what does:
>
> javaclasspath
>
> return?


Here it is:


octave:2> chk_spreadsheet_support("/usr/share/java", [])
ans =  78
octave:3> javaclasspath
   STATIC JAVA PATH

      - empty -

   DYNAMIC JAVA PATH

      /usr/share/java/poi-4.0.1.jar
      /usr/share/java/poi-ooxml-4.0.1.jar
      /usr/share/java/xmlbeans-3.0.2.jar
      /usr/share/java/poi-ooxml-schemas-4.0.1.jar
      /usr/share/java/dom4j-2.1.1.jar
      /usr/share/java/commons-collections4.jar
      /usr/share/java/commons-compress.jar
      /usr/share/java/jxl-2.6.12.jar
      /usr/share/java/xercesImpl-2.12.0.jar
      /usr/share/java/xml-apis-1.3.04.jar
      /usr/share/java/jOpenDocument-1.3-1.3repack.jar


Sébastien Villemot <svillemot>
Thu 15 Oct 2020 03:58:32 PM UTC, comment #3: 

chk_spreadsheet_support.m's return value of 78 is what you'd get with JXL (8), full POI (2 + 4) and JOD (64) support (+ OCT) and 74 is the same with basic POI support(just .xls).
4 is the added value for POI-OOXML; see "help chk_spreadsheet_support".

Again, what does:

javaclasspath

return?

(just to get that bit out of the way. For me as maintainer and potential bug-solver that does matter. That's not meant to challenge your POV; probably like you, I think the actual bug here lies elsewhere.)

I'm currently investigating why xlsopen didn't fall back to OCT as you didn't select an explicit interface and from the messages in your comment #0, OCT should have been the only active interface that supports .xlsx.

Thanks.

<title adapted>

Philip Nienhuis <philipnienhuis>
Group Member
Thu 15 Oct 2020 12:36:34 PM UTC, comment #2: 

commentaire #1 :

> In your post I do not see "POI (& OOXML)" in xlsopen.m's output but just "POI". That implies that only .xls support for POI is active, not .xlsx. What does your javaclasspath look like after the call to chk_spreadheet_support? BTW the ouput of chk_spreadsheet_support is a bit mask indicating which interfaces have been successfully checked. 74 as in your post hints at incomplete POI initialization.


For my Debian packaging work, I use several containers, which don’t necessarily have the same set of Java libraries installed. Hence the discrepancy between the body of my post (bitmask 74) and the log file I attached (bitmask 78). The problem occurs in both cases.

> On my box I see Java complaints during POI initialization about a "reflective error" or so (bug #58266) and the threat to disable that in future Java implementations. Do you have newer Java where they actually did so?


I don’t see that error message. All my tests were performed using Debian unstable, which defaults to OpenJDK 11.

> In the entries for OCT there's a "subscripts must be either integers 1 to (2^63)-1 or logicals" error. That leads me to the following:
> Can you try replacing L.131 in xlopen.m,
>       reqinterface = [];
> by
>       reqinterface = "";
> try again and and report back?


I applied this changes and the problem persists (I attach the log, the summary table at the end is the same as the previous one, but the error messages slightly differ, as you can see with a text diff tool).


(file #49982)

Sébastien Villemot <svillemot>
Thu 15 Oct 2020 12:16:23 PM UTC, comment #1: 

Thanks for the report, we already had some limited private discussion.

Afer installing io using -forge, I could reproduce the bug on Windows, but erratically.
On my Linux system (Mageia 7) I can not, it all works fine there.

You've attached the output of "test_spsh.m", one of the dedicated spreadsheet I/O test scripts, thanks.
That script also shows that operation is erratic. JXL and POI don't work, OCT partly and JOD works fine.

In your post I do not see "POI (& OOXML)" in xlsopen.m's output but just "POI". That implies that only .xls support for POI is active, not .xlsx. What does your javaclasspath look like after the call to chk_spreadheet_support? BTW the ouput of chk_spreadsheet_support is a bit mask indicating which interfaces have been successfully checked. 74 as in your post hints at incomplete POI initialization.

On my box I see Java complaints during POI initialization about a "reflective error" or so (bug #58266) and the threat to disable that in future Java implementations. Do you have newer Java where they actually did so?

In the entries for OCT there's a "subscripts must be either integers 1 to (2^63)-1 or logicals" error. That leads me to the following:
Can you try replacing L.131 in xlopen.m,
      reqinterface = [];
by
      reqinterface = "";
try again and and report back?

Philip Nienhuis <philipnienhuis>
Group Member
Thu 15 Oct 2020 09:37:52 AM UTC, original submission:  

Hi,

After a call to chk_spreadsheet_support, loading XLSX files no longer work in 2.6.2. Here is an example session:


octave:1> pkg load io
OpenJDK 64-Bit Server VM warning: Archived non-system classes are disabled because the java.system.class.loader property is specified (value = "org.octave.OctClassLoader"). To use archiv
ed non-system classes, this property must be not be set
octave:2> chk_spreadsheet_support("/usr/share/java", [])
ans =  74
octave:3> cd ~/debian/octave-io/debian/tests/
octave:4> xlsfinfo('test.xlsx', [], true)
Detected interfaces: POI; JXL; JOD;
None.
warning: xlsopen: no'.xlsx' spreadsheet I/O support with available interfaces.
octave:5>


Note that the problem did not occur with 2.6.1.

Also note that XLSX loading works fine if chk_spreadsheet_support is not called.

I also attach a diary file showing the output of test_spsh.

Sébastien Villemot <svillemot>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #50042:  __OCT_spsh_open__.m added by philipnienhuis (8KiB - application/octet-stream)
file #50043:  __ods_get_sheet_dims__.m added by philipnienhuis (6KiB - application/octet-stream)
file #50033:  __POI_spsh_open__.m added by philipnienhuis (2KiB - application/octet-stream)
file #50003:  xlsopen.m added by philipnienhuis (17KiB - application/octet-stream)
file #50004:  getinterfaces.m added by philipnienhuis (8KiB - application/octet-stream)
file #49985:  xlsopen.m added by philipnienhuis (17KiB - application/octet-stream)
file #49986:  getinterfaces.m added by philipnienhuis (8KiB - application/octet-stream)
file #49981:  io.log added by svillemot (5KiB - text/x-log)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by svillemot (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 18 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-10-24 philipnienhuis StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2020-10-21 philipnienhuis Attached File- Added _OCT_spsh_open_.m, #50042
        Attached File- Added _ods_get_sheet_dims_.m, #50043
    2020-10-21 svillemot Attached File- Added octave-io_2.6.2-2~1.gbpe0ea3c_amd64-2020-10-21T12:05:22Z.build, #50034
    2020-10-20 philipnienhuis Attached File- Added _POI_spsh_open_.m, #50033
    2020-10-20 svillemot Attached File- Added octave-io_2.6.2-2~1.gbpe0ea3c_amd64-2020-10-20T13:50:07Z.build, #50028
    2020-10-17 philipnienhuis Attached File- Added xlsopen.m, #50003
        Attached File- Added getinterfaces.m, #50004
    2020-10-16 philipnienhuis StatusConfirmed In Progress
    2020-10-15 philipnienhuis Attached File- Added xlsopen.m, #49985
        Attached File- Added getinterfaces.m, #49986
    2020-10-15 philipnienhuis Summary[octave forge] (io) can't load XLSX files after chk_spreadsheet_support in 2.6.2 [octave forge] (io) can't reliably load XLSX files in release 2.6.2
    2020-10-15 svillemot Attached File- Added io-xlsopen-patched-L131.log, #49982
    2020-10-15 philipnienhuis StatusNone Confirmed
        Assigned toNone philipnienhuis
        Operating SystemGNU/Linux Any
    2020-10-15 svillemot Attached File- Added io.log, #49981

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code