Bug 106529 - LibO 5.3.1.x breaks extensions for changes in BASIC parser (error message: "Basic Syntax error. Expected: ).")
Summary: LibO 5.3.1.x breaks extensions for changes in BASIC parser (error message: "B...
Status: NEW
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Extensions (show other bugs)
Version:
(earliest affected)
5.3.1.1 rc
Hardware: All All
: high normal
Assignee: Not Assigned
QA Contact:
URL:
Whiteboard: target:5.3.3 target:5.4.0 target:5.3.2
Keywords: regression
: 106549 106685 106687 106869 (view as bug list)
Depends on:
Blocks:
 
Reported: 2017-03-14 10:13 UTC by sergio.callegari
Modified: 2017-03-31 14:29 UTC (History)
14 users (show)

See Also:
Crash report or crash signature:


Attachments
Addon for trying. (507.10 KB, application/vnd.openofficeorg.extension)
2017-03-14 13:02 UTC, Dorange-Pattoret Didier
Details
Fixed Basic modules (72.20 KB, application/zip)
2017-03-25 21:05 UTC, Andreas Säger
Details

Note You need to log in before you can comment on or make changes to this bug.
Description sergio.callegari 2017-03-14 10:13:17 UTC
Extension TexMaths which provides an alternate and much more feature full equation editor for Libreoffice seems to be broken in 5.3. Libreoffice now seems to complain about functions with missing ")" which seems to be related to a different way of parsing the basic macros. Seen in 5.3.1.2 on Linux.
Comment 1 Dorange-Pattoret Didier 2017-03-14 13:02:04 UTC
Created attachment 131879 [details]
Addon for trying.
Comment 2 Dorange-Pattoret Didier 2017-03-14 13:05:17 UTC
I confirm for LO 5.3.1.2 (Win64, Win32 and MacOs64).
I join an addon to test.
Thanks.
Comment 3 Dorange-Pattoret Didier 2017-03-14 13:06:28 UTC
To test : http://www.dmaths.org/tele.php?f=OOoGdmath.oxt
Comment 4 Jacques Guilleron 2017-03-14 13:34:29 UTC
Hi sergio, Didier,

I tried these extensions(TexMaths and OOoGdmath) and got the opening of the Basic wizard on the closing parenthesis missing since:

LO 5.3.1.1 Build ID: 72fee18f394a980128dc111963f2eefb05998eeb
Threads CPU : 2; Version de l'OS :Windows 6.1; UI Render : par défaut; Moteur de mise en page : nouveau; Locale : fr-FR (fr_FR); Calc: CL

These extensions were working until:
LO 5.3.0.3 Build ID: 7074905676c47b82bbcfbea1aeefc84afe1c50e1
Threads CPU : 2; Version de l'OS :Windows 6.1; UI Render : par défaut; Moteur de mise en page : nouveau; Locale : fr-FR (fr_FR); Calc: CL

Jacques
Comment 5 Jacques Guilleron 2017-03-14 13:53:29 UTC
Severity should be increased to Major, since a lot of extensions cannot be installed in LO 5.3.1
Comment 6 tommy27 2017-03-14 14:20:49 UTC
so it's a regression introduced in 5.3.1.x?
was it working in 5.3.0, right?
Comment 7 Katarina Behrens (CIB) 2017-03-14 19:11:12 UTC
'twas bug#80731 - missing closing parenthesis in Basic expressions is now considered to be an error (which it really is) instead of being silently ignored.

So yes, the issues mentioned here are bugs, but bugs in the extensions, not in Libreoffice core code. 

Please have extension authors fix their code.
Comment 8 sergio.callegari 2017-03-15 13:41:13 UTC
> missing closing parenthesis in Basic expressions is now considered to be an error (which it really is)

Hard not to agree, yet:

Is there any good reason why a change with the potential to break extensions has gone into a point release? According to wikipedia, which I think is copying over the LibO policy, "[LibO] Releases are designated by three numbers separated by dots. The first number is the major version (branch) number, the second one usually indicates small changes, and the final one /bugfixes/."

Can this be pushed back for 5.4, having the 5.3.x point releases from now on carry a clear deprecation statement in the release notes?

> Please have extension authors fix their code

Hard not to agree again, but without a clear deprecation note it will leave users needing the extensions on "stable" rather than "fresh" until the extensions are fixed, which will leave users with a codebase with less features and ultimately make "fresh" less tested (possibly leading to a more buggy "stable" when "fresh" is promoted)
Comment 9 tommy27 2017-03-15 14:45:35 UTC
I agree with Sergio.

this fix, though correct from a technical point of view, will probably create more negative effects from a user point of view.

It would be probably better to post-pone it to 5.4.x and alert extension creators that requirement changed and that they have to fix their own codes to make it functional in LibO
Comment 10 Katarina Behrens (CIB) 2017-03-16 14:28:29 UTC
> Is there any good reason why a change with the potential to break extensions
> has gone into a point release?

The original author of the patch (Pierre Lepage) committed the change to master ( = 5.4) only. It was someone else who cherry-picked it for 5.3.1 and yet someone else who reviewed it in ...

https://gerrit.libreoffice.org/#/c/33341

So let's ask Caolan what his intention was ...

> Can this be pushed back for 5.4, having the 5.3.x point releases from now on
> carry a clear deprecation statement in the release notes?

You mean if this can be reverted for 5.3.x?
Comment 11 tommy27 2017-03-16 14:54:44 UTC
(In reply to Katarina Behrens (CIB) from comment #10)
>  ...
> 
> > Can this be pushed back for 5.4, having the 5.3.x point releases from now on
> > carry a clear deprecation statement in the release notes?
> 
> You mean if this can be reverted for 5.3.x?

yes, I mean to keep the patch in 5.4.x and revert it in 5.3.x to avoid breakage of extensions and give more time to extension authors to fix their code before 5.4.x
Comment 12 sergio.callegari 2017-03-16 15:06:31 UTC
> You mean if this can be reverted for 5.3.x?

If the patch breaks extensions, having a "deprecation period" before the "quirk" the extensions relied upon is removed would seem good practice to me. In this sense, reverting it in 5.3.x, while leaving it in 5.4 while adding a visible deprecation note to the 5.3 release notes and a similarly visible note about the parser change in the 5.4 release notes (as they are being built) would seem fit.

But I do not know if the patch was pushed to 5.3.x as an effective bugfix for something (because not doing so immediately would have left broken some core parte of LibO). In the latter case, the balance would change in favor of leaving the patch in the 5.3.x series.

As another ingredient to the discussion I can point out that the TeXMaths authors is being extremely responsive in working to fix his widely used extension. I am already testing a pre-release of a new version which addresses the problem.

Still, I do not know if there are other "published" extensions affected. Furthermore, there may be users relying on "unpublished" extensions (i.e., extensions they had someone develop for them only to address some need) that may be affected.
Comment 13 Caolán McNamara 2017-03-16 15:36:09 UTC
re https://gerrit.libreoffice.org/#/c/33341 I don't remember any specific reason for backporting it to 5.3, maybe the author asked how to get it into 5.3 or something like that, I don't know of anything that will immediately break if it goes the other direction.
Comment 14 tommy27 2017-03-17 04:46:13 UTC
maybe too late to revert the patch in 5.3.1
that should be probably done in 5.3.2
Comment 15 Katarina Behrens (CIB) 2017-03-17 09:21:35 UTC
> But I do not know if the patch was pushed to 5.3.x as an effective bugfix
> for something (because not doing so immediately would have left broken some
> core parte of LibO).

Nah, as you can see, it was more like "this fixes a bug and looks otherwise harmless, let's backport it". Some core parts of LibO were left broken, but only as aftermath (as they were not free of missign-closing-parenthesis errors either) 
 
> Still, I do not know if there are other "published" extensions affected.
> Furthermore, there may be users relying on "unpublished" extensions (i.e.,
> extensions they had someone develop for them only to address some need) that
> may be affected.

And not only extensions, I guess there's now thousands of macros that don't work anymore
Comment 16 tommy27 2017-03-17 10:24:38 UTC
(In reply to Katarina Behrens (CIB) from comment #15)
> ...
> 
> And not only extensions, I guess there's now thousands of macros that don't
> work anymore

it sounds horrible...
let's consider a patch revert and an extra 5.3.1 rc3 then...
Comment 17 Katarina Behrens (CIB) 2017-03-17 14:30:10 UTC
> it sounds horrible...
> let's consider a patch revert and an extra 5.3.1 rc3 then...

So I've reverted. For 5.3.1 we're sadly too late to a party (released yesterday)
Comment 18 tommy27 2017-03-20 17:27:27 UTC
(In reply to Katarina Behrens (CIB) from comment #17)
> > it sounds horrible...
> > let's consider a patch revert and an extra 5.3.1 rc3 then...
> 
> So I've reverted. For 5.3.1 we're sadly too late to a party (released
> yesterday)

ok, does this mean that it's FIXED with target 5.3.2.x?
Comment 19 Johnny_M 2017-03-20 21:11:03 UTC
> ok, does this mean that it's FIXED with target 5.3.2.x?

It's pending review: https://gerrit.libreoffice.org/35340
Comment 20 Commit Notification 2017-03-21 11:46:34 UTC
Katarina Behrens committed a patch related to this issue.
It has been pushed to "libreoffice-5-3":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=660e394b893a045a077ccded263f94a1be03fcf9&h=libreoffice-5-3

tdf#106529: Revert "tdf#80731 Closing parenthesis is now detected"

It will be available in 5.3.3.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 21 Mike Kaganski 2017-03-22 04:02:50 UTC
*** Bug 106687 has been marked as a duplicate of this bug. ***
Comment 22 Jim DeLaHunt 2017-03-22 06:21:28 UTC
Is there anything in the BASIC parser which _warns_ about syntax errors, like missing parentheses, short of throwing an error and making the Extension fail?  It would be good to have a middle ground: a warning which the Extension developer can see, letting them know there's a problem to fix, while not breaking the Extension for all its users.  Contrariwise, if the BASIC parser without this patch does NOT complain, does it mean that the syntax is actually correct, and will continue to work even with a stricter parser?
Comment 23 Jim DeLaHunt 2017-03-22 06:24:33 UTC
By the way, after bug 106687 occurred I noticed that I was not able to edit the failing macro in my macro editor. I'm not sure this is part of this bug or not. 

If it's the case that the macro editor is not capable of editing a macro which has the "missing closing parenthesis" error, after the parser throws its syntax error, then the parser is creating a situation where it's hard to correct buggy macros.  

When this syntax check is applied again, I would suggest a test which confirms that macros with a variety of syntax errors are still editable in the Macro editor.
Comment 24 Jean-Baptiste Faure 2017-03-22 08:01:12 UTC
(In reply to Jim DeLaHunt from comment #23)
> By the way, after bug 106687 occurred I noticed that I was not able to edit
> the failing macro in my macro editor. I'm not sure this is part of this bug
> or not. 

I disagree: I had the same problem with the Altsearch extension and the Basic editor was opened with the offended code allowing me to fix the bug for myself. That said the editor was opened behind the current window and it was not clear with the error message only that the user can do something to fix the problem.
It would be better if the error message dialog had a button to open the Basic editor at the line containing the error.

Best regards. JBF
Comment 25 Xisco Faulí 2017-03-22 09:35:50 UTC
*** Bug 106685 has been marked as a duplicate of this bug. ***
Comment 26 Katarina Behrens (CIB) 2017-03-22 16:48:52 UTC
So I wonder if we can coordinate the activity somewhere, which extensions are fixed or at the least their author(s) are notified and which are still missing 

As in 5.3.3 (possibly 5.3.2 already) the errors in Basic code will be silently ignored again and only couple of brave souls actually uses extensions w/ unstable 5.4, hardly ever will we learn there's actually an issue.

I've seen TexMath has a ticket (and possibly also a fix already), anyone else?
Comment 27 sergio.callegari 2017-03-22 16:53:20 UTC
I confirm that TeXMaths is affected and has a fixed version that is ready (but not yet officially announced and released)
Comment 28 Jim DeLaHunt 2017-03-22 18:49:43 UTC
(In reply to Jean-Baptiste Faure from comment #24)
> (In reply to Jim DeLaHunt from comment #23)
> > By the way, after bug 106687 occurred I noticed that I was not able to edit
> > the failing macro in my macro editor. I'm not sure this is part of this bug
> > or not. 
> 
> I disagree: I had the same problem with the Altsearch extension and the
> Basic editor was opened with the offended code allowing me to fix the bug
> for myself....

JBF, thank you for your report.

Let me be clear, on my system (LO 5.3.1.2 on Mac OS X 10.11 El Capitan), LO opened the Basic editor, had an orange arrow pointing to the line with the error, but did not let me make any changes to the code.  The Macro editor was read-only for me.

JBF, what LO version and OS are you using?  Maybe I am seeing a problem limited to the Mac build of LO.
Comment 29 Jim DeLaHunt 2017-03-22 21:17:37 UTC
OK, I've done a careful check. I think JBF may have been correct. However, I think this may be a different Extensions Manager bug.

Procedure 1:
Install Typography toolbar extension, for All Users
Restart Libreoffice
Create new Writer document with "text123679"
Change text to Linux Libertine G font.
View... Toolbars... Typography... Make toolbar visible in window.
Select text.
Click "Oldstyle figures" button in Typography toolbar.
BASIC Syntax error occurs.
My Macros & Dialogs.typo appears.
"typo" is listed under "My Macro & Dialogs" not under "LibreOffice Macros & Dialogs"
Line of sub prg, with syntax error, indicated by orange arrow.
Result 1: Not able to edit the contents of the macro.

Procedure 2:
Install Typography toolbar extension, click button "Only for me"
Then same as Procedure 1.
Result 1: Yes, able to edit the contents of the macro.

Test system:
LibreOffice 5.3.1.2, Mac OS X El Capitan 10.11.6, Typography toolbar extension 1.1.

Conclusion 1: Jean-Baptiste Faure (from comment #24) is correct, the Macro Editor does not permit editing macros installed for All Users. My problem editing is not evidence of a connection to the syntax error or the macro editor state after a syntax error.

Conclusion 2: it is a problem that the Macro editor silently refuses to edit, rather than giving some positive indication that editing is not permitted. It could have the UI say "Read-only". It could put up an error alert. It could have a different background colour.

Conclusion 3: it is strange to me that the typo macro, when installed for "All Users", appears under "My Macros & Dialogs". I would expect it to appear under "LibreOffice Macros & Dialogs".

I will look into filing Conclusions 2 and 3 as an Extensions Manager or Macro Editor bug.
Comment 30 Katarina Behrens (CIB) 2017-03-22 23:04:02 UTC
> Conclusion 2: it is a problem that the Macro editor silently refuses to
> edit, rather than giving some positive indication that editing is not
> permitted. It could have the UI say "Read-only". It could put up an error
> alert. It could have a different background colour.

This is a long-standing issue -- if the macro is read-only, there's no indication whatsoever (e.g. this yellow-ish "This document is read-only" info bar normal documents have) that this is the case. I'm sure it is already filed somewhere.
Comment 31 Xisco Faulí 2017-03-22 23:27:55 UTC
*** Bug 106710 has been marked as a duplicate of this bug. ***
Comment 32 Mike Kaganski 2017-03-23 05:32:51 UTC
(In reply to Katarina Behrens (CIB) from comment #26)
> So I wonder if we can coordinate the activity somewhere, which extensions
> are fixed or at the least their author(s) are notified and which are still
> missing 
> ...
> I've seen TexMath has a ticket (and possibly also a fix already), anyone
> else?

Typography extension developer (Lazlo Nemeth) is notified and answered that he'll look at this.
Comment 33 Mike Kaganski 2017-03-23 05:48:21 UTC
AltSearch author (Tomáš Bílek) has been sent an email.
Comment 34 Mike Kaganski 2017-03-23 05:52:21 UTC
... as well as author of DCM Direct Colour Management - Gerhard Weydt.
Comment 35 Mike Kaganski 2017-03-23 06:07:46 UTC
Read Text extension: a new issue filed https://github.com/jimholgate/readtextextension/issues/9
Comment 36 Mike Kaganski 2017-03-23 06:53:22 UTC
To conclude the problematic extensions known at this moment:

- access2base - the author (Jean-Pierre Ledure) is subscribed to tdf#106710.
- decroise' author is informed and is said to work on it (tdf#106676).
Comment 37 Dorange-Pattoret Didier 2017-03-23 15:25:55 UTC
Hello,
I corrected the code in two extensions Gdmath and Dmaths.
I added the missing ).
It works fine.
Yhanks for your job.
Comment 38 Gerhard Weydt 2017-03-23 15:38:24 UTC
Concerning extension DCM:
I supplied the missing parentheses and tested with rel. 5.3.1.2: the error no longer occurred after the changes.
I added a new release to my extension, but it does not show there as the current release. It is visible at the bottom of the list, nevertheless, which is out of the normal order. I can download and install it, and it works properly.
So the remaining problem is that it should be displayed as the current release. Can anybody help?
Comment 39 Andreas Säger 2017-03-24 20:08:38 UTC
Almost every Library shipped with LibreOffice shows the same problem as you can prove by brwosing module by module and click the compile button.
Comment 40 Mike Kaganski 2017-03-25 04:43:32 UTC
(In reply to Andreas Säger from comment #39)

I assume that you took the effort and found some places with modules shipped with LO? So why not to tell exactly where did you find them, instead of posting uninformative messages that essentially say "I found them but not telling where; go and find yourself". And better yet, file a dedicated bug for that, because that's part of LO (unlike external extensions), and should be tracked by LO developers. The new bug report should reference this one in its See Also.
Comment 41 Buovjaga 2017-03-25 10:02:33 UTC
Andreas: see previous comment.
Comment 42 Andreas Säger 2017-03-25 21:05:08 UTC
Created attachment 132144 [details]
Fixed Basic modules
Comment 43 Andreas Säger 2017-03-25 21:06:19 UTC
I'll attach a zip with all fixed Basic modules in ${INSTALLDIR}/basic/.
I appended a closing brace to all lines where the new Basic compiler complained since this is what the old compiler assumed when one was missing.

Library.Module  Line_Numbers_in_Basic_IDE
------------------------------------------
Access2Base.Application 917,1149
Access2Base.Database 1443,
Access2Base.Event 255,
Access2Base.Recordset 1120
Access2Base.Trace 183
Access2Base.Utils 832
Depot.Currency 73
Depot.Internet 207
FormWizard.DBMeta 289,290
FormWizard.FormWizard 137,140,141
FormWizard.Language 48,//113,118,140,141,142,143
Gimmicks.ReadDir 247
------------------------------------------------

FormWizard.Language had a comment starting with // in line 113.
Syntax highlighting indicates that this is a valid comment. But the compiler does not accept this line. I've never seen a //comment in Basic.
Comment 44 Andreas Säger 2017-03-25 21:07:32 UTC
Sorry, the correct base dir is ${INSTALLDIR}/share/basic/
Comment 45 Mike Kaganski 2017-03-26 20:41:23 UTC
(In reply to Andreas Säger from comment #43)

access2base is fixed in bug 106710. Gimmicks fixed by commit 6bb6ca1fb30f786385c2357e5435077066a49f82.

The other fixes are submitted to gerrit in https://gerrit.libreoffice.org/35726. Andreas, could you please send your license statement (as described on https://wiki.documentfoundation.org/Development/Developers), that is required to merge your contribution? Thank you!
Comment 46 Andreas Säger 2017-03-26 23:46:49 UTC
(In reply to Mike Kaganski from comment #45)
> (In reply to Andreas Säger from comment #43)
> 
> access2base is fixed in bug 106710. Gimmicks fixed by commit
> 6bb6ca1fb30f786385c2357e5435077066a49f82.
> 
> The other fixes are submitted to gerrit in
> https://gerrit.libreoffice.org/35726. Andreas, could you please send your
> license statement (as described on
> https://wiki.documentfoundation.org/Development/Developers), that is
> required to merge your contribution? Thank you!

Done
Comment 47 Commit Notification 2017-03-27 04:36:07 UTC
Andreas Säger committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=ad4857211bcc39ccd4c84c670bfab1b937f65089

tdf#106529: fix closing parentheses of bundled macros

It will be available in 5.4.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 48 Commit Notification 2017-03-27 06:59:03 UTC
Andreas Säger committed a patch related to this issue.
It has been pushed to "libreoffice-5-3":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=7fb7b091e963d8c45abf1db1202a88f2e188e9b0&h=libreoffice-5-3

tdf#106529: fix closing parentheses of bundled macros

It will be available in 5.3.3.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 49 Julien Nabet 2017-03-27 11:49:43 UTC
*** Bug 106549 has been marked as a duplicate of this bug. ***
Comment 50 Commit Notification 2017-03-27 12:58:43 UTC
Katarina Behrens committed a patch related to this issue.
It has been pushed to "libreoffice-5-3-2":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=4725c2a282f2c6468125073ba9cb5ba66f488e08&h=libreoffice-5-3-2

tdf#106529: Revert "tdf#80731 Closing parenthesis is now detected"

It will be available in 5.3.2.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 51 Xisco Faulí 2017-03-30 12:07:54 UTC
*** Bug 106869 has been marked as a duplicate of this bug. ***
Comment 52 Volga 2017-03-31 10:33:29 UTC
This bug also affect Writer2ePub (https://extensions.libreoffice.org/extensions/writer2epub/1-1-2.8)

Version: 5.3.2.1 (x64)
Build ID: 7f6693c08cc110b9721245fc4bd4f1712e0c086c
CPU Threads: 4; OS Version: Windows 6.19; UI Render: default; Layout Engine: new; 
Locale: zh-CN (zh_CN); Calc: CL

And AltSearch extension put a bugfix release 1.4.2 to work around this bug.
Comment 53 Commit Notification 2017-03-31 14:29:25 UTC
Katarina Behrens committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=351dead74b4c213b13102f81b5ae9bb47ad8ca39

tdf#106529: Revert "tdf#80731 Closing parenthesis is now detected"

It will be available in 5.4.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.