Issue 121754 - Build HSQLDB with JDK 7
Build HSQLDB with JDK 7
Status: RESOLVED FIXED
Product: Base
Classification: Application
Component: code
3.4.1
All All
: P3 minor (vote)
: 4.0.0
Assigned To: Andrea Pescetti
:
Depends on:
Blocks: 121690
  Show dependency treegraph
 
Reported: 2013-02-11 17:40 UTC by Rob Weir
Modified: 2013-08-03 08:05 UTC (History)
9 users (show)

See Also:
Issue Type: PATCH
Latest Confirmation on: ---
Developer Difficulty: ---
jsc: 4.0.0_release_blocker-


Attachments
Buildable patch (8.85 KB, patch)
2013-02-20 05:09 UTC, Ariel Constenla-Haile
no flags Details | Diff
Update HSQLDB to version 1.8.0.11, released specifically to address this issue (4.00 KB, patch)
2013-06-30 14:58 UTC, Andrea Pescetti
no flags Details | Diff
Patch committed in r1500167 (21.35 KB, patch)
2013-07-06 06:36 UTC, Andrea Pescetti
no flags Details | Diff
remove the failiing part (1.35 KB, patch)
2013-07-09 13:20 UTC, Regina Henschel
no flags Details | Diff
Patch committed in r1501409 (17.25 KB, patch)
2013-07-09 18:29 UTC, Andrea Pescetti
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this issue.
Description Rob Weir 2013-02-11 17:40:00 UTC
Patches attached to this email post on the Dev mailing list:

http://markmail.org/message/j6nmujh5476rftsv
Comment 1 Ariel Constenla-Haile 2013-02-20 05:07:13 UTC
(In reply to comment #0)
> Patches attached to this email post on the Dev mailing list:
> 
> http://markmail.org/message/j6nmujh5476rftsv

That mail has no attachment.
The patch should attached to this bug, and should be buildable (the patch copied in that mail is made from the unpacked sources; the developer should make a patch, put it on trunk/main/hsqldb/patches and add it to trunk/main/hsqldb/makefile.mk, *then* generate the patch).
Comment 2 Ariel Constenla-Haile 2013-02-20 05:09:06 UTC
Created attachment 80307 [details]
Buildable patch

Based on http://markmail.org/message/j6nmujh5476rftsv
Comment 3 Fred Toussi 2013-02-20 11:11:13 UTC
This patch simply adds stubs for new methods added to java.sql package interfaces in Java 7.

However, if the sources are compiled with JDK7, you will need JDK7 on the target machine to run AOO. The java.sql.* JDBC library interfaces are linked at compile time and force the runtime requirement for the JDK version that compiled the code or a higher version.
Comment 4 Ariel Constenla-Haile 2013-02-20 11:28:11 UTC
(In reply to comment #3)
> This patch simply adds stubs for new methods added to java.sql package
> interfaces in Java 7.
> 
> However, if the sources are compiled with JDK7, you will need JDK7 on the
> target machine to run AOO. The java.sql.* JDBC library interfaces are linked
> at compile time and force the runtime requirement for the JDK version that
> compiled the code or a higher version.

That's why I think it shouldn't be committed. The Java baseline in OpenOffice is still 1.5 (both source and target) and there are no plans to change this, so a proper fix would be making the code compile with JDK7 while generating binaries that run in 1.5.

I've raised this concern several times (for example http://markmail.org/message/imckfah34tsfhfrs ) but people seems to misunderstand it, and still talk about "dropping java6" (whatever that could mean).
Comment 5 Ariel Constenla-Haile 2013-02-20 11:29:38 UTC
Adapted the bug title.
Comment 6 Fred Toussi 2013-02-20 12:04:09 UTC
> That's why I think it shouldn't be committed. The Java baseline in
> OpenOffice is still 1.5 (both source and target) and there are no plans to
> change this, so a proper fix would be making the code compile with JDK7
> while generating binaries that run in 1.5.

Unfortunately it seems impossible to compile classes that implement java.sql package interfaces to force a previous JRE target. Therefore a baseline JDK 5 is required for release builds.

But there is no harm in committing the patch (after some mods) just to make it simpler for developers. Developers can already compile with JDK 6 as support is built into the existing HSQLDB sources.

The patch needs changes to //#ifdef and //#endif lines as they must start at the beginning of the line (example below)

+ public class jdbcConnection implements Connection {
++//#ifdef JAVA7
 
The build.xml must also be updated to include a target for JDK7, something like:

<target name="switchtojdk17" depends="switchtojdk16" ...

You can look at build.xml for hsqldb 2.2.x for an example.
Comment 7 Andrea Pescetti 2013-02-26 23:06:53 UTC
I confirm that this patch allows building hsqldb with a Java 7 compiler. As clarified by Pedro on the dev list, we don't have a more comprehensive Java 7 patch.

So what's preventing this from being committed? That the generated binaries would not run with Java 5/6? Would Fred's latest comment address this? If it is an elaborate issue, feel free to raise it on the dev list.
Comment 8 Fred Toussi 2013-02-26 23:59:05 UTC
> That the generated binaries would not run with Java 5/6? Would Fred's latest comment address this? 

With the patch applied in its present form, the sources would not compile with JDK 5/6. HSQLDB has a preprocessor that comments in/out #ifdef tagged code that is valid/not valid for the JDK used for compile. The tags must be in the right place and the build.xml must be upgraded for JDK7. These changes are quite simple.
Comment 9 Ariel Constenla-Haile 2013-02-27 00:02:05 UTC
(In reply to comment #7)
> I confirm that this patch allows building hsqldb with a Java 7 compiler. As
> clarified by Pedro on the dev list, we don't have a more comprehensive Java
> 7 patch.
> 
> So what's preventing this from being committed? 

The patch is incomplete and some errors, see comment 6 from Fred (who happens to be the HSQLD developer, so he knows what he is talking about ;) ). Pedro pointed to the FreeBSD solution: 
http://www.freebsd.org/cgi/cvsweb.cgi/ports/editors/openoffice-3-devel/files/
which seems to be correct, as it also modifies build.xml, but the "debian" in the name of the patches suggests they took the patches from Debian, so the precedence/license is unclear.

Note that I was/am/will not working on this; I simply made the patch from the copy&paste in the mailing list because they pinged a developer to look at it.

@folling: are you working on this?

I posted detailed instructions how to generate a patch that works in the build environment: http://markmail.org/message/gh5caocay7wao5g2
Comment 10 Fred Toussi 2013-02-27 11:26:58 UTC
An updated build.xml has been committed to HSQLDB SVN.

http://hsqldb.svn.sourceforge.net/viewvc/hsqldb/base-one/tags/hsqldb_1_8_0_11/build/build.xml?revision=5193&content-type=text%2Fplain

The patch for jdbcDataSource is incorrect. The inserted code stub must be moved further down (outside the existing #ifdef /#else tags).

We I have time, I will add the Java 7 method stubs to the HSQLDB hsqldb_1_8_0_11 SVN repository, which can then be used to patch hsqldb_1_8_0_10
Comment 11 Fred Toussi 2013-02-28 21:48:02 UTC
Updated sources and build.xml file for HSQLDB 1.8.0.11 are avaiable from the HSQLDB project SVN. See here:

http://hsqldb.svn.sourceforge.net/viewvc/hsqldb/base-one/tags/hsqldb_1_8_0_11/

These files compile correctly with JDK7.
Comment 12 Andrea Pescetti 2013-03-10 14:37:14 UTC
So, to make sure I understood this right, all relevant patches are now incorporated into HSQLDB 1.8.0.11 and it would be enough to update HSQLDB from 1.8.0.10 to 1.8.0.11 in the OpenOffice external source dependencies?
Comment 13 Rob Weir 2013-03-11 15:02:10 UTC
I'm adding this comment to all open issues with Issue Type == PATCH.  We have 220 such issues, many of them quite old.  I apologize for that.  

We need your help in prioritizing which patches should be integrated into our next release, Apache OpenOffice 4.0.

If you have submitted a patch and think it is applicable for AOO 4.0, please respond with a comment to let us know.

On the other hand, if the patch is no longer relevant, please let us know that as well.

If you have any general questions or want to discuss this further, please send a note to our dev mailing list:  dev@openoffice.apache.org

Thanks!

-Rob
Comment 14 Andrea Pescetti 2013-03-19 23:01:00 UTC
Answering Rob: yes, this bug should be fixed in version 4. The original patch by follinge, as modified by Ariel, works for me. But from the comment above I understand that updating HSQLDB to 1.8.0.11 (which might imply rebasing some patches, I haven't checked) should be the best solution.

Anyway, this issue should be fixed in 4.0 by integrating the patch or (probably better) by updating to  HSQLDB 1.8.0.11.

[Setting target 4.0, feel free to reset if we haven't agreed how to set it yet]
Comment 15 Fred Ollinger 2013-06-12 21:49:01 UTC
I have not been working on this. I just wanted to get things working with a new version of java.

I have been going back and forth with people on which versions of java are "supported". I'm still not clear on this. I just figured that it made since to build on newer versions of java so people with modern distros could build with a clean install of a newer linux distro.

I can go back and put ifdefs in this, but I think that the correct decision is to NOT apply this patch and instead to update hsqldb if it will work on all versions of java. Plus, it should actually have the functions implemented instead of stubs which I planned on implementing if and when things became an issue.
Comment 16 Kay 2013-06-12 23:13:27 UTC
I think it is vital to do a build that will enable acceptable java 1.7+ usage by end users. We simply can not tell them to use java 1.6- with the security problems that exist in that old environment. 


If we can build with 1.6 but users can run acceptably with 1.7 and above that would be ideal. Apache buildbots are still at 1.6, with linux buildbot at openJDK 6.


We appreciate continued assistance in this area.
Comment 17 Andrea Pescetti 2013-06-30 14:58:36 UTC
Created attachment 80961 [details]
Update HSQLDB to version 1.8.0.11, released specifically to address this issue

The attached patch updates HSQLDB to version 1.8.0.11, thus enabling to build the "hsqldb" module with both Java 6 and Java 7 (I don't have older versions at the moment). System requirements for users are not expected to be affected by this patch.

Note that we download the package from OOo Extras, and that the package there is manually obtained from
http://hsqldb.svn.sourceforge.net/viewvc/hsqldb/base-one/tags/hsqldb_1_8_0_11/
since
http://sourceforge.net/projects/hsqldb/files/hsqldb/hsqldb_1_8_0/
does not offer version 1.8.0.11 at the moment. The package contents with respect to 1.8.0.10 are significantly different in size, so I will replace this 1.8.0.11 package with a more "official" version if that appears in the HSQLDB files area at SourceForge.

Also note that two out of the four patches currently at
http://svn.apache.org/viewvc/openoffice/trunk/main/hsqldb/patches/
get removed since they are already included in HSQLDB 1.8.0.11.
Comment 18 Kay 2013-06-30 22:16:19 UTC
Andrea --

Ok, I see the move to:

http://code.google.com/a/apache-extras.org/p/ooo-extras/downloads/list

Will you be updating:

/main/external-deps.lst

as well?
Comment 19 Andrea Pescetti 2013-07-01 05:16:30 UTC
(In reply to Kay from comment #18)
> Will you be updating:
> /main/external-deps.lst
> as well?

The patch already does it, see the first chunk at
https://issues.apache.org/ooo/attachment.cgi?id=80961&action=diff
It is not committed yet, but complete.
Comment 20 Kay 2013-07-01 15:29:32 UTC
OK, sorry for the confusion on my part. I see this now.

Time to apply and see what happens. Thanks.
Comment 21 jsc 2013-07-02 07:14:54 UTC
why is this not already checked in and tested for a while? If it is not critical and don't break the build with older Java versions.

What's the current status exactly, doe sit build on all our major platforms, Linux, Mac, Windows, ...?
Comment 22 Kay 2013-07-02 22:03:12 UTC
I'm assuming at this point, we should just use Andrea's patches only?

otherwise this doesn't make sense as Ariel's patch included i121754.patch on hslqdb and this is not mentioned in Andrea's makefile.mk.
Comment 23 Andrea Pescetti 2013-07-03 05:51:21 UTC
What I can say is that the patched code did build hsqldb on both Java 6 and Java 7 (Linux, 64 and 32 bit respectively).

@fredt: I'm perplexed by comparing your comment here
https://issues.apache.org/ooo/show_bug.cgi?id=121754#c6
(where you mention that 'target name="switchtojdk17"' must be added)
to the revision log of
http://hsqldb.svn.sourceforge.net/viewvc/hsqldb/base-one/tags/hsqldb_1_8_0_11/build/build.xml?view=log
(where you added it in http://hsqldb.svn.sourceforge.net/viewvc/hsqldb/base-one/tags/hsqldb_1_8_0_11/build/build.xml?r1=3266&r2=5193 but you then removed it in http://hsqldb.svn.sourceforge.net/viewvc/hsqldb/base-one/tags/hsqldb_1_8_0_11/build/build.xml?r1=5193&r2=5194 ). Is this as expected?
Comment 24 Fred Toussi 2013-07-03 08:31:36 UTC
(In reply to Andrea Pescetti from comment #23)
> @fredt: I'm perplexed by comparing your comment here
I simplified the added code in the SVN to avoid using new #ifdef blocks. The SVN head you are looking at will compile correctly on Java 5 as well as later versions.

I would like to incorporate the remaining patches at
http://svn.apache.org/viewvc/openoffice/trunk/main/hsqldb/patches/
but I will probably use slightly different code. If this will not cause confusion then I can do it today. Then you can retire those patches and use a diff between 1.8.0.10 release code and 1.8.0.11 SVN for the patch.

Andrea, please get in touch direct to iron this out and and get it done quickly.
Comment 25 Kay 2013-07-03 15:58:28 UTC
in response to Andrea #23:

Yes, I also incorporated the IFDEF statements for a build in April, but it seems there were execution issues with java 7. However, the other patches you now have here were not in place. So, I'll see.
Comment 26 SVN Robot 2013-07-05 23:16:25 UTC
"pescetti" committed SVN revision 1500167 into trunk:
#i121754# Patch HSQLDB to align it to version 1.8.0.11, enable building on Ja...
Comment 27 Andrea Pescetti 2013-07-06 06:36:19 UTC
Created attachment 81010 [details]
Patch committed in r1500167

Some more details:

- Fred Toussi (thanks) made a newer 1.8.0.11 release (newer than the previous 1_8_0_11 tag) available at http://hsqldb.svn.sourceforge.net/viewvc/hsqldb/base-one/tags/hsqldb_1_8_0_11/ ; but this was not released in ZIP form, so the previous idea of simply updating the ZIP file is not the best.

- The 1_8_0_11 tag supersedes the four patches we used to apply and enables building with Java 7.

- The attached patch, as agreed with Fred, takes care of updating the current 1.8.0.10 with all code changes done in the 1_8_0_11 tag. To create it, I started with the ZIP version of 1.8.0.10 and compared all files to the 1_8_0_11 tag. There were dozens of false positives due to the "$Id$" tags, whitespace changes and other similar differences. I only kept files that were significantly different, plus one new file (src/org/hsqldb/lib/StringComparator.java) that is in 1_8_0_11 but not in the 1.8.0.10 ZIP file. So the final result should be that all non-trivial (whitespace, comments...) changes from 1_8_0_11 are backported.

- This was tested by building with Java 6 and Java 7; the hsqldb module built successfully on both systems.
Comment 28 Kay 2013-07-07 21:10:36 UTC
OK, I'm having problems building -- have junit enabled for QA build -- and stops in the same place -- 

complex/connectivity/HsqlDriverTest.java:34: error: package org.hsqldb.lib does not exist
import org.hsqldb.lib.StopWatch;
                     ^
complex/connectivity/hsqldb/TestCacheSize.java:36: error: package org.hsqldb.lib does not exist
import org.hsqldb.lib.StopWatch;

The current deliverd hslqdb with latest snapshot contains lib/Stopwatch.class

So, some questions -- I have your latest patches in my working copy.

And, it seems current main/external_deps.lst has this:

if (SOLAR_JAVA == TRUE)
    MD5 = 17410483b5b5f267aa18b7e00b65e6e0
    name = hsqldb_1_8_0.zip
    URL1 = http://sourceforge.net/projects/hsqldb/files/hsqldb/hsqldb_1_8_0/hsqldb_1_8_0_10.zip/download
    URL2 = $(OOO_EXTRAS)$(MD5)-$(name)

If this is correct, then something is wrong -- where is org.hsqldb.lib.StopWatch.class?
Comment 29 Fred Toussi 2013-07-08 10:55:03 UTC
(In reply to Kay from comment #28)
> where is org.hsqldb.lib.StopWatch.class?
The class is in hsqldb.jar.

The hsqldb.jar must be built before the test classes. This jar must be included in the classpath when the test classes are built
Comment 30 Kay 2013-07-08 16:14:40 UTC
re Fred's comment --

Yes. My conclusion is that in my recent build, hsqldb was either not built at all (likely since I don't see it anywhere in the build tree), or my build sequence is faulty. Will investigate by trying manual build with the provided ant build.xml.
Comment 31 jsc 2013-07-09 09:21:28 UTC
what is the current status of this issue now? It seems to be really a blocker at the moment.
Comment 32 Regina Henschel 2013-07-09 13:20:15 UTC
Created attachment 81042 [details]
remove the failiing part

The part, where the readme file is patched, does not apply on Windows but produces a build break. The attached patch removes that part.

I wonder, that the file i121754.patch has DOS line ends (and so has this patch). In addition I currently struggle with git. So please have a look and help fixing it on master.
Comment 33 Regina Henschel 2013-07-09 15:18:31 UTC
All-clear for line ends, trunk is OK, but I have to repair my local copy.
Comment 34 Andre 2013-07-09 16:12:03 UTC
Looks like the root cause of the non-applying patch is a DOS line start, ie a carriage return at the start of a line.  This is not handled by the patch command.
I played around with the patch (helped by the Emacs Diff mode) and when the offending line is left untouched then the whole patch applies without further problems.

With this idea (not changing the line with the ^M at the beginning) the resulting readme file would look like this:

    Readme File
    June 2013

    ^MThis package contains HSQLDB 1.8.0.10
    This package contains HSQLDB 1.8.0.11 (please ignore the line above).
    ...
Comment 35 Regina Henschel 2013-07-09 16:22:31 UTC
When I examine i121754.patch with a hex editor, I see mixed line ends in the file. It starts with 0A but later on a lot of 0D0A exists. So my previous statement "trunk is OK regarding line ends" might be wrong.
Comment 36 Kay 2013-07-09 17:01:16 UTC
Good detective work, Regina -- hex editors come in VERY handy for this type of thing. No wonder patch is having problems. At the very least, I think we should change the patch to consistent line ends. 

I wonder how the buildbot is getting past this issue. Maybe setup to just try ANY kind of line end, I don't know.
Comment 37 SVN Robot 2013-07-09 17:57:26 UTC
"pescetti" committed SVN revision 1501409 into trunk:
#i121754# Simplify the patch to avoid line-end problems.
Comment 38 Andrea Pescetti 2013-07-09 18:29:16 UTC
Created attachment 81043 [details]
Patch committed in r1501409

Status: this new patch removes all ^M characters (line-ends problems) and only keeps the code differences, thus removing readme and HTML files. If you examine the options given to diff, you'll see that I had to fiddle with line-ends due to the fact that the two versions have different line-end conventions. So the patch was not mixing line-ends per se, it was a diff between files with different line-ends conventions.

Tested like the other time: hsqldb module built successfully under Java 6 and 7 (Linux). Regina, thanks for your feedback and if you now can build on Windows too then this should be fine.
Comment 39 Regina Henschel 2013-07-10 06:53:44 UTC
I have build successfully on Windows 7 with Java 6.
Comment 40 jsc 2013-07-11 11:23:09 UTC
is it possible to update the status of this issue

MacOS and Windows builds with "release build options" are ok
Comment 41 Kay 2013-07-11 19:57:59 UTC
Still not building hsqldb for me on rev 1502186 with openjdk7 in Linux.

I will investigate further.
Comment 42 Andre 2013-07-12 08:05:55 UTC
@Kay: Can you describe you current build problem?
Comment 43 Kay 2013-07-12 16:06:46 UTC
Basically, the build gets down to building "connectivity", which depends on hsqldb being built, but hsqldb is NOT built, that is for certain! No errors or warnings occur for the hsqldb build -- if it'd even invoked correctly -- so that's all I can tell you at the moment. I was going to do a build --from hsqldb to see what would happen. At this point, due to recent changes on my system, I don't know if it's an ant problem -- maybe? -- or the new java (openJDK 7 instead of Oracle java 7 <--- this one worked ok on older build in April) or what.

So, more information when I know more.
Comment 44 jsc 2013-07-15 11:17:52 UTC
remove showstopper request because we currently build our release not with Java 7

For 4.1 I propose to switch completely to Java 7 and change our build dependencies accordingly.

But for now this is no stopper issue for me.
Comment 45 Andrea Pescetti 2013-07-15 17:21:02 UTC
This issue was fixed on trunk before the AOO400 release branch was created:
http://svn.apache.org/viewvc?view=revision&revision=1501409

So it is fixed in 4.0 too: we can build OpenOffice 4 with Java 7 (with HSQLDB 1.8.0.10 patched to incorporate all the few code changes from 1.8.0.11).

For 4.1 we should look into moving to HSQLDB 2.x, but there is already an issue (and an old CWS) for that.

@Kay: since you never managed to successfully finish the build before or after the fix, I suggest that you open a separate issue and put me in CC.