Comment 1 Ferenc Hechler 2008-02-19 20:32:10 EST

Created attachment 90132 [details]
patch for org.eclipse.jdt.ui, add Jar-in-Jar ClassLoader (without zip-resource)

Comment 2 Ferenc Hechler 2008-02-19 20:34:42 EST

Created attachment 90133 [details]
Zip-Resource to be put under ui/ord.eclipse.jdt.internal.ui.jarpackagerfat

Comment 3 Ferenc Hechler 2008-02-19 20:37:01 EST

Created attachment 90134 [details]
zipped sources for the rsrc-url-loader_142.zip (JarRsrcLoader)

Comment 4 Ferenc Hechler 2008-02-21 03:08:32 EST

Created attachment 90300 [details]
Zip-Resource to be put under ui/ord.eclipse.jdt.internal.ui.jarpackagerfat 

fixed problems with jdk <1.6 (wrong Manifest was opened [rt.jar])
removed String.split() for compatibility with jdk1.3.1,
compiled with jdk 1.3.1

Comment 5 Ferenc Hechler 2008-02-21 03:10:35 EST

Created attachment 90301 [details]
zipped sources for the rsrc-url-loader_142.zip (JarRsrcLoader)

fixed problems with jdk <1.6 (wrong Manifest was opened [rt.jar])
removed String.split() for compatibility with jdk1.3.1

Comment 6 Ferenc Hechler 2008-02-21 12:14:40 EST

There is a NPE for external jars in the build-path.
Will be fixed with next patch

Comment 7 Benno Baumgartner 2008-02-22 07:11:24 EST

(In reply to comment #1)
> Created an attachment (id=90132) [details]
> patch for org.eclipse.jdt.ui, add Jar-in-Jar ClassLoader (without zip-resource)

Ooops, I'm sorry, my commit of bug 213638 made this patch invalid...

Let us just see if I understand that correct: When executing the generated jar file then JarRsrcLoader.main(String[]) is executed. This main method replaces the standard class loader with the URLClassLoader and delegates to the 'real' main class? Is that correct?

Times up for 3.4 legal reviews. So in order to get that patch in for 3.4 we need a small patch, the patch must be around 300 lines of code, I think that's doable, I see a lot of duplicate and unused code:

- Is the change in IJarBuilder really required? jarFile.getName() returns the path to the jar file...
- It looks like the FatJarRsrcUrlManifestProvider copies code from ManifestProvider, try to inherit/find common abstraction
- What does Rsrc mean?
- We need unit tests...
- nextNumberedJarName is copied in FatJarRsrcUrlBuilder and FatJarRsrcUrlManifestProvider, merge them, move to FatJarUtils? What is this method doing anyway?
- Never write to standard out: 
                try {
			writeRsrcUrlClasses();
		} catch (IOException e) {
			e.printStackTrace();
			throw new CoreException(null);
		}
- Make FatJarRsrcUrlBuilder a subclass of FatJarBuilder
- We can not just put a zip file in the jarpackagerfat package. Well, we could, but this is not good: We need to add the source for the url-loader to cvs somehow: We need versioning for this code. We need to be able to fix it. If we fix something in the code the fixed code must be in the next build. We can not create the zip resource from hand each time we fix something. We would need an ant script that generates the zip resource on each build. I don't like this idea at all. Can we add the source to the jarpackagerfat package? And the FatJarRsrcUrlBuilder reads and copies the class files to the jar file? I think that should be possible. It would solve this problem.

Let me know when you think your code is ready for a review. M6 is feature freeze btw...

Comment 8 Ferenc Hechler 2008-02-22 08:01:33 EST

Hi Benno,

thanks for your review.
Below are my answers:

> Ooops, I'm sorry, my commit of bug 213638 made this patch invalid...
I am not in a Hurry!  :)
So lets wait, until the create-ant script contribution is finished and then create a new patch based on this sources.

> Let us just see if I understand that correct: 
> [...] Is that correct?
yes. The Information for the real Main-Class and included jar-libs are read from the manifest of the outer jar (Attributes "Rsrc-Main-Class" and "Rsrc-Class-Path")

> Times up for 3.4 legal reviews. 
> [...]
The patch is a sort of quick-hack for a proof of concept.
I am very proud, that the new class-loader is able to handle signer-files 
and it is only a little extension to the standard techniques.

> - Is the change in IJarBuilder really required? 
>   jarFile.getName() returns the path to the jar file...
Yep, so we can leave the interface unchanged. Did not see this!  :)

> - It looks like the FatJarRsrcUrlManifestProvider copies code from
>   ManifestProvider, try to inherit/find common abstraction
currently I have not much time, it was the fastest way to get it up and running.

> - What does Rsrc mean?
Short for "Resource". 
"rsrc:pkg/test.txt" is the content accessible as Original-ClassLoader.getResource("pkg/test.txt").
Additionally "rsrc:pkg/" references the folder "pkg/" inside the outer jar. So we can acces both types (libs and class-folders) inside a JAR:
1.) "jar:rsrc:lib/inner.jar!/" references the contents of inner.jar
2.) "rsrc:subf/" references the classes beyond the subfolder "subf" inside the JAR.

> - We need unit tests...
:-)

> - nextNumberedJarName is copied in FatJarRsrcUrlBuilder and
> FatJarRsrcUrlManifestProvider, merge them, move to FatJarUtils? 
> What is this method doing anyway?
It prevents problems with jars which have the same name (in different folders).

> - Never write to standard out: 
Just for me to debug, forgot to remove this.

> - Make FatJarRsrcUrlBuilder a subclass of FatJarBuilder
I will look wether an abstract base class or direct inheritence might be better.

> - We can not just put a zip file in the jarpackagerfat package. 
> [...]
I used the ZIP file to be able to compile it with JDK 1.3.1 for compatibility reasons when launching the compiled jar. Do you see another possibility to compile classes with an old JDK?

> Let me know when you think your code is ready for a review. 
> M6 is feature freeze btw...
If it gets into 3.5 it is ok...


feri

Comment 9 Benno Baumgartner 2008-02-22 09:52:34 EST

(In reply to comment #8)
> > Let me know when you think your code is ready for a review. 
> > M6 is feature freeze btw...
> If it gets into 3.5 it is ok...

Ok, shall we set the target to 3.5 therefore?

Comment 10 Ferenc Hechler 2008-02-22 11:22:20 EST

Created attachment 90475 [details]
patch for org.eclipse.jdt.ui, add Jar-in-Jar ClassLoader (with zip-resource)

new version based on current CVS HEAD (with ANT-Script contribution).

Comment 11 Ferenc Hechler 2008-02-22 11:23:19 EST

Created attachment 90477 [details]
patch for org.eclipse.jdt.ui, add Jar-in-Jar ClassLoader (without zip-resource)

new version based on CVS-HEAD with ANT Export

Comment 12 Ferenc Hechler 2008-02-22 11:24:25 EST

Created attachment 90478 [details]
Zip-Resource to be put under ui/org.eclipse.jdt.internal.ui.jarpackagerfat

compiled with jdk 1.3.1

Comment 13 Ferenc Hechler 2008-02-22 11:25:42 EST

Created attachment 90479 [details]
zipped sources for the jar-rsrc-loader_131.zip (JarRsrcLoader)

sources compatible with jdk 1.3.1

Comment 14 Ferenc Hechler 2008-02-22 11:29:00 EST

Created attachment 90480 [details]
zipped sources for the jar-rsrc-loader_131.zip (JarRsrcLoader)

previous attachment was wrong (the binary file)

Comment 15 Ferenc Hechler 2008-02-22 12:32:28 EST

Also changed back IJarBuilder interface, as Benno stated.
This also resolves the problems with external libs.

Comment 16 Ferenc Hechler 2008-02-22 16:48:01 EST

Created attachment 90522 [details]
patch for org.eclipse.jdt.ui, add Jar-in-Jar ClassLoader (without zip-resource)

inherit FatJarRsrcUrl-Classes from FatJar-Classes,
save/restore widget-value.

Comment 17 Ferenc Hechler 2008-02-23 09:48:53 EST

Created attachment 90546 [details]
Zip-Resource to be put under ui/org.eclipse.jdt.internal.ui.jarpackagerfat

removed some try/catch/printStackTrace/System.out ...

Comment 18 Ferenc Hechler 2008-02-23 09:50:13 EST

Created attachment 90547 [details]
zipped sources for the jar-rsrc-loader_131.zip (JarRsrcLoader)

removed some try/catch/printStackTrace/System.out ...

Comment 19 Ferenc Hechler 2008-02-23 10:06:48 EST

Created attachment 90548 [details]
patch for org.eclipse.jdt.ui, add Jar-in-Jar ClassLoader (without zip-resource)

Inheritance, inheritance, ...   :)
Changed inherited values for merge-manifest and remove-signers from true to false, because that is not necessary for Jar-in-Jar.

Comment 20 Ferenc Hechler 2008-02-23 17:40:38 EST

Created attachment 90562 [details]
patch for org.eclipse.jdt.ui, add Jar-in-Jar ClassLoader (without zip-resource)

Added FatJarRsrcUrlAntExporter for exporting ANT scripts including the Jar-in-Jar loader.

Comment 21 Benno Baumgartner 2008-02-25 04:17:00 EST

Sorry Ferenc, I was wrong: M6 is API freeze and M7 is feature freeze. That means we have time until M7 but only if:
1. We can do it without API addition/change
2. We can do it without a legal review

1. shouldn't be a problem, but I'm not sure about 2. M7 is Friday May 2, 2008

Comment 22 Ferenc Hechler 2008-02-25 04:27:17 EST

> ...M7 is feature freeze. That means we have time until M7 but only if:
> 1. We can do it without API addition/change
> 2. We can do it without a legal review
>
> 1. shouldn't be a problem, but I'm not sure about 
> 2. M7 is Friday May 2, 2008

about 1.) - I agree, we can do it without any API changes. The new patch has only the internal package "jarpackegrfat".

about 2.) - What are the conditions to circumvent a legal-review? How are the 300 Lines of code counted? We have the Jar-Loader-Code, the Patch and Test-code. Are comment-lines also counted?

Comment 23 Benno Baumgartner 2008-02-25 05:18:08 EST

(In reply to comment #22)
> about 2.) - What are the conditions to circumvent a legal-review? How are the
> 300 Lines of code counted? We have the Jar-Loader-Code, the Patch and
> Test-code. Are comment-lines also counted?

"Big chunks of code from contributors (and, of course, initial contributions to projects) must be reviewed and approved by Eclipse Legal."

I think the idea is, that small bug fixes/enhancements do not require a review. I have not a very good feeling about releasing such a feature without an IP review. And you said that it is ok for you to wait for 3.5. So, I would like to move this to 3.5. OK?

Another question: The signing of the jars is kept, right? But are the signs also verified at runtime?

Comment 24 Ferenc Hechler 2008-02-25 06:26:41 EST

>So, I would like to move this to 3.5. OK?

ok, thats fine.

> Another question: The signing of the jars is kept, right? 
> But are the signs also verified at runtime?

That is the main advantage of the jar-loader.
Even JCE Provider which undergo some security-checks can be used.
I have tested it with the "BouncyCastle" and "CryptixCrypto" Provider and no Security Exception!

Comment 25 Ferenc Hechler 2008-03-13 12:21:24 EDT

Created attachment 92469 [details]
patch for org.eclipse.jdt.ui, add Jar-in-Jar ClassLoader (without zip-resource)

merged with bugfix 220257 and added testcases for jar-in-jar loader

Comment 26 Benno Baumgartner 2008-06-27 09:07:26 EDT

*** Bug 237439 has been marked as a duplicate of this bug. ***

Comment 27 Benno Baumgartner 2008-07-03 11:02:01 EDT

Hi Ferenc

I would now like to request legal approval for your patch if you are still interested. But first, I've found some minor issues:

- There is a TODO in FatJarRsrcUrlBuilder
- Add contributor info to all changed/created files
- If the Jar-in-Jar option is used then I think we should not show the warning, that the operation repacks libraries. We certainly must not say that signature files are not copied.
- There are a lot of warnings and some spelling errors in the source for the jar in jar class loader
- Is the jar-rsrc-loader_131.zip file really required? I've added the source for the class loader to the package. Can't you read the generated class files from this package? This way we don't need to rebuild the zip file when we fix something in the class loader...
- I'm not convinced about the UI, Jar-in-Jar loader is rather technical. I think it would be better to express, that either the required libraries are extracted or not. Maybe something like:

Library handling:
 ( ) Extract required libraries into generated JAR
 (o) Copy required libraries into generated JAR

We could even show the repack and missing signature files warning on the dialog as soon as the user selects the first option.

Benno

Comment 28 Benno Baumgartner 2008-07-03 11:04:48 EDT

Created attachment 106450 [details]
patch against I20080702-0939

Comment 29 Benno Baumgartner 2008-07-03 11:09:04 EDT

(In reply to comment #27)
> - Is the jar-rsrc-loader_131.zip file really required? I've added the source
> for the class loader to the package. Can't you read the generated class files
> from this package? This way we don't need to rebuild the zip file when we fix
> something in the class loader...

Sorry, we've already discussed that:

(In reply to comment #8)
> > - We can not just put a zip file in the jarpackagerfat package. 
> > [...]
> I used the ZIP file to be able to compile it with JDK 1.3.1 for compatibility
> reasons when launching the compiled jar. Do you see another possibility to
> compile classes with an old JDK?

No, I don't.

Comment 30 Ferenc Hechler 2008-07-03 13:58:10 EDT

Hello Benno,

of course I am still interested in adding this Feature.
Till July, 14th I am on vaccation and have only unfrquent access to the Internet.
I will take a look at the issues You mentioned.

Best regards, 
    feri

Comment 31 Benno Baumgartner 2008-07-08 04:12:17 EDT

(In reply to comment #30)
> Till July, 14th I am on vaccation and have only unfrquent access to the
> Internet.

Sure, no problem, we have a lot of time.

Regarding the zip file, we see 4 possibilities:
1. Keep it as it is at the moment, which requires to recompile and rebuild the zip from hand every time we fix something in the class loader.
2. Add a new project to the eclipse repository owned by jdt/ui with a 1.3 compliance.
3. Keep the code in the jdt/ui plugin and add to the readme that the jar-in-jar jars can only be executed with a JVM 1.4+
4. Build the classloader from source when exporting the jar with the compliance of the project which is exported.

2. is too much paperwork, no really. I could live with 3., who uses a 1.3 VM anyway? Or is it required to be 1.3 compliance even with newer VMs? I could live with 1. because I don't think that we need to fix this code often and we have unit test which fail if the zip is not correctly build. But if you implement 4. that would awesome! I also think that this would be an interesting task.

Comment 32 Ferenc Hechler 2008-07-18 18:11:13 EDT

Created attachment 107872 [details]
 patch for org.eclipse.jdt.ui, add Jar-in-Jar ClassLoader (without zip-resource)

Comment 33 Ferenc Hechler 2008-07-18 18:14:53 EDT

Created attachment 107873 [details]
 Zip-Resource to be put under ui/org.eclipse.jdt.internal.ui.jarpackagerfat

Comment 34 Ferenc Hechler 2008-07-18 18:36:53 EDT

Attachment 106450 [details] "patch against I20080702-0939" should be obsoleted by Attachment 107872 [details], but I have no rights to do this.

the following changes have been made:

 * added contributor info to all changed/created files
 * replaced Check-Box Use-Jar-In-Jar with radio-buttons for Library-Handling
 * Repack-Warning is shown as soon as the Extract-Jars library handling option is selected
 * fixed some warnings and spelling errors (update jar-rsrc-loader_131.zip)

> There is a TODO in FatJarRsrcUrlBuilder
This is still an open issue.
There is a problem reading the jardesc file. 
There are two different builder classes depending on the selected library handling ("FatJarBuilder" and "FatJarRsrcUrlBuilder"). 

Comment 35 Ferenc Hechler 2008-07-22 02:49:24 EDT

>> There is a TODO in FatJarRsrcUrlBuilder
>This is still an open issue.
>There is a problem reading the jardesc file. 
Jardesc files are not supported with the Runnable Jar File export wizard, so perhaps this issue can be ignored.

The optional IPWarning dialog logic is buggy, will fix this.

Comment 36 Ferenc Hechler 2008-07-22 03:07:30 EDT

The "Repacking Libraries Warning" logic is already buggy in Eclipse 3.4

1.) Export a project which uses a library.
2.) Click "Finish"
2.) The IP-Issue Dialog appears.
3.) Activate the checkbox "Do not show this message again" 
4.) Click "Cancel" 
5.) The Export is cancelled and the wizard page is active
6.) Again click "Finish"
7.) The export is started without querying the message again.

Is this behaviour correct?
I assume that cancelling the dialog should not persist the "Do not show again" selection.

Comment 37 Benno Baumgartner 2008-07-22 03:25:03 EDT

(In reply to comment #36)
> Is this behaviour correct?
> I assume that cancelling the dialog should not persist the "Do not show again"
> selection.

Yes, you're right, this is not correct. The checkbox state must not be persisted when canceling.

Comment 38 Ferenc Hechler 2008-07-22 15:40:15 EDT

Created attachment 108121 [details]
patch for org.eclipse.jdt.ui, add Jar-in-Jar ClassLoader (without zip-resource)

Fixes the problem with the optional repackaging dialog. 
Cancelling the Dialog sets the show-dialog-status to enabled.

Comment 39 Ferenc Hechler 2008-07-22 19:16:51 EDT

Perhaps we could add a third Library-Handling option for unpackaged runnable jars as mentioned in Bug 224732 ( https://bugs.eclipse.org/bugs/show_bug.cgi?id=224732 )

Library handling:
 ( ) Extract required libraries into generated JAR
 (o) Copy required libraries into generated JAR
 ( ) Copy required librarids into subfolder lib/

The Class-Path could be set accordingly ("lib/jar1.jar lib/jar2.jar ...")

Best regards,

    feri

Comment 40 Ferenc Hechler 2008-07-28 12:56:30 EDT

Created attachment 108552 [details]
 patch for org.eclipse.jdt.ui, add Jar-in-Jar ClassLoader (without zip-resource)

Added third export method 

 * Copy files into subfolder

Which creates a runnable-jar and copies all libs into a subfolder named in the wizard-dialog.

No Subfolder-Handling like check, overwriting, ... is done.

ANT export is also missin.


This is a first "proof of concept" implementation to be discussed.

Perhaps it might be better to create a ZIP file containing the JAR and the libs. Of course the zip is not executable, but it can be used for distributions.

Comment 41 Benno Baumgartner 2008-07-29 04:06:21 EDT

(In reply to comment #40)
> This is a first "proof of concept" implementation to be discussed.

Hi Ferenc

This patch does not work for me: (exporting http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.swt.snippets/src/org/eclipse/swt/snippets/Snippet245.java?view=co with SWT Library on classpath)

NPE when building jar-in-jar, nothing when building fatjar and lib-jar. The patch also has conflicts with HEAD.

Regarding the UI:
- Maybe add a separator on top of 'Library handling'? Maybe groups would look good?
- Maybe don't allow to specify the name of the libraries folder, always use lib? Keep it simple...
- Do not pop up a dialog when selecting a radio button. What I thought is to show the warning message inline in the dialog, like on the 'Sort Members' dialog.

BTW, I would not be surprised if we need to show a warning for the jar-in-jar packager as well: I'm not sure if it is allowed to pack a LGPL library in such a way. But I'm not a lawyer...

Comment 42 Ferenc Hechler 2008-07-29 05:24:28 EDT

Created attachment 108619 [details]
patch for org.eclipse.jdt.ui, add Jar-in-Jar ClassLoader (without zip-resource)

> NPE when building jar-in-jar, 
> nothing when building fatjar and lib-jar. 
> The patch also has conflicts with HEAD.
oops, I had no conflicts. 
Updated to HEAD and created new patch.
Snippet245 runs in all three modes.

feri

Comment 43 Dani Megert 2008-07-31 04:01:23 EDT

*** Bug 224732 has been marked as a duplicate of this bug. ***

Comment 44 Ferenc Hechler 2008-08-03 17:25:15 EDT

Created attachment 109021 [details]
patch for org.eclipse.jdt.ui, add Jar-in-Jar ClassLoader (without zip-resource)

Simplified the dialog:

Library handling:

( ) Extract required libraries into generated JAR
( ) Package required libraries into generated JAR
(x) Zip required libraries together with generated JAR

No Subfolder selection has to be done. 
Inside the ZIP all libraries are in the subfolder (lib/). 
Using ZIP has the advantage of not disturbing other builds in the same directory.

Perhaps it should be checked wether there are any libs or not.

Technically: 
A temporary JAR file (named as given in the "export destination") is generated "C:/xx/test.jar"
Also a ZIP-File is created with the same-name only with changed extension "c:/xx/test.zip". The temporary JAR and all other libraries are added.
Afterwards the temporary JAR file is deleted.

If files with the same name already exist, the user is asked, whether to overwrite or not.

The IP-Issue-Warning dialog has been removed when selecting the "Extract" option. Instead of this a tool-tip text has been added.
The Warning appears after pressing the "Finish" button as before.

ANT-Scripts can be generated for all three options.

Comment 45 Benno Baumgartner 2008-08-04 03:47:20 EDT

(In reply to comment #44)
> (x) Zip required libraries together with generated JAR

Oh, but I liked it more the way it was before: Copy files into subfolder. I think on *nix people would rather like to have a .tar.gz file and on OSX a .dmg file. Now they need to unzip the generated file before they can pack the application they way they like to.

Comment 46 Ferenc Hechler 2008-08-04 04:51:29 EDT

My motivation to use the ZIP was to circumvent problems with unused libraries or libraries from other exports to the same folder (which could have the same name but different content). 
The ZIP file is deleted every time the export is started and all old stuff is gone.  :)

What about creating a subfolder named after the runnable-jar with a suffix and putting all jars there together. Example:

Export destination: "path/myRunApp.jar"

output:

"path/myRunApp_export/myRunApp.jar"
"path/myRunApp_export/lib/anylib1.jar"
"path/myRunApp_export/lib/anylib2.jar"
...

For the second export:
We would ask the user to delete the content beyond "path/myRunApp_export" and then recreate all files.

Comment 47 Ferenc Hechler 2008-08-04 05:15:43 EDT

> We would ask the user to delete the content 
> beyond "path/myRunApp_export" and then recreate all files.
I meant:
We would ask the user for permisson to delete the old content...

Comment 48 Benno Baumgartner 2008-08-04 05:55:55 EDT

(In reply to comment #46)
> Export destination: "path/myRunApp.jar"
> output:
> "path/myRunApp_export/myRunApp.jar"

Mmm, no, if I select the thing to be exported to path/myRunApp.jar I expect it to be there, and not somewhere else. Why not ask the user for permission to delete the /lib folder if one already exists?

Comment 49 Ferenc Hechler 2008-08-04 06:30:44 EDT

> Why not ask the user for permission to
> delete the /lib folder if one already exists?
> 
I would not dare to delete any ones lib/ folder even if he gave me the permission. Think about the project-root dir.  ;)

We could include the jar-name as prefix for the lib-subfolder:

  "path/myRunApp_lib/"

This also solves the problem of multiple exports to the same dir.
Also this should be easy to understand for the user.

Comment 50 Benno Baumgartner 2008-08-04 06:40:07 EDT

(In reply to comment #49)
> We could include the jar-name as prefix for the lib-subfolder:
>   "path/myRunApp_lib/"

OK, sounds good.

Comment 51 Ferenc Hechler 2008-08-04 14:08:19 EDT

Created attachment 109085 [details]
patch for org.eclipse.jdt.ui, add Jar-in-Jar ClassLoader (without zip-resource)

Removed automatic zipping. 
The third export option is now:

(x) Copy required libraries into a sub-folder next to the generated JAR

Libraries are copied in a subfolder "xxx_lib" where "xxx" is the name part of the export destination ("path/xxx.jar") without the extension.
Before any copying is done the whole sub-folder is erased. In case of problems an error is thrown.

The ANT export was not changed (still creates a ZIP).

Comment 52 Ferenc Hechler 2008-08-05 06:19:30 EDT

Created attachment 109144 [details]
patch for org.eclipse.jdt.ui, add Jar-in-Jar ClassLoader (without zip-resource)

Added ANT export for "Copy to sub-folder" option.

Changed message for query overwrite folder dialog:
  The folder 'xxx_lib' already exists.
  Do you want to overwrite all content?

Comment 53 Benno Baumgartner 2008-08-14 04:05:09 EDT

(In reply to comment #52)
> Created an attachment (id=109144) [details]
> patch for org.eclipse.jdt.ui, add Jar-in-Jar ClassLoader (without zip-resource)

Ferenc? Is this patch your final version? Then we could start to integrate this, and ask for legal review.

Comment 54 Ferenc Hechler 2008-08-14 04:15:02 EDT

I was not sure if the created structure is ok for all.

But if there are no comments about that I will look over the code and write some Unit-Tests for the subfolder export.

Then a legal review can be started.

Comment 55 Ferenc Hechler 2008-08-18 16:38:33 EDT

Created attachment 110285 [details]
patch for org.eclipse.jdt.ui, add Jar-in-Jar ClassLoader (without zip-resource)

Tests:
- Added tests for subfolder export library handling.
- increased timeout-values for launching runnable jars from 3 to 5 seconds

Export-Wizard:
- Changed allowOverwrite handling for subfolder-export (do not ask for permission during test-run).
- Added code from bug 243163 (create dir-entries in generated JAR).

Next I will do some pretty printing of the code and look for lost TODOs. Then a legal review can be done.

Comment 56 Ferenc Hechler 2008-08-19 03:37:54 EDT

Created attachment 110315 [details]
patch for org.eclipse.jdt.ui, add Jar-in-Jar ClassLoader (without zip-resource)

sorry, in the last patch the package org.eclipse.jdt.ui.jarpackager was missing.

Comment 57 Ferenc Hechler 2008-08-19 14:07:49 EDT

Created attachment 110378 [details]
patch for org.eclipse.jdt.ui, add Jar-in-Jar ClassLoader (without zip-resource) 2008-08-19

 - Added text "jar-rsrc-loader_131_howto_create.txt" describing how to build the binary
 - Added bug-info in header
 - removed all warnings

This patch can be reviewed.

Comment 58 Benno Baumgartner 2008-08-21 11:43:21 EDT

(In reply to comment #57)
> This patch can be reviewed.

- This patch contains also the fix for bug 243163, please provide a patch without the fix for bug 243163, we don't need legal review for this.
- The 'how to create zip' file does not explain how to create the zip file. A batch file would also be helpful.
- The first time (fresh workspace) the runnable jar wizard opens none of the library handling radios is selected.
- The library handling label should be aligned with the other labels in the dialog.
- When exporting with copy to subfolder a dialog ask if I want to create the subfolder bla_lib. Do I really have a choice? I guess not. Just create it, I mean you also don't ask if I want to create bla.jar, right?
- Don't use this static library handling constants in FJPWP. Instead define an abstract class (LibraryHandling?) and implement methods getAntExporter, getJarBuilder, showIPIssue,... for each kind of library handling. This way it is much easier to add a new kind of library handling because you don't need this switch statements anymore. In general: replace switches with polymorphism in OOP. 

Other then that it looks good!

Comment 59 Ferenc Hechler 2008-08-22 10:07:30 EDT

Created attachment 110677 [details]
patch for org.eclipse.jdt.ui, add Jar-in-Jar ClassLoader (without zip-resource) 2008-08-22

Beside the redesign of the library handling all issues mentioned by benno are done:

 - removed fix for bug 243163
 - added an ANT build script to create the jar-rsrc-loader.zip (runnable from inside the src-package
 - renamed jar-rsrc-loader_131.zip to jar-rsrc-loader.zip. The new binary will be attached separate.
 - initial library handling is set to EXTRACT (blank workspace)
 - label "Library Handling" is aligned with other labels
 - the subfold_lib is created without asking the user

The library handling will be redesigned next

Comment 60 Ferenc Hechler 2008-08-22 10:10:28 EDT

Created attachment 110678 [details]
Zip-Resource to be put under ui/org.eclipse.jdt.internal.ui.jarpackagerfat 2008-08-22

The new binary built using build_jar-rsrc-loader.xml

Comment 61 Ferenc Hechler 2008-08-22 17:43:50 EDT

Created attachment 110721 [details]
patch for org.eclipse.jdt.ui, add Jar-in-Jar ClassLoader (without zip-resource) 2008-08-22b

refactored library handling

Comment 62 Ferenc Hechler 2008-08-25 02:48:41 EDT

Created attachment 110774 [details]
patch for org.eclipse.jdt.ui, add Jar-in-Jar ClassLoader (without zip-resource) 2008-08-25

Once again refactored library handling. Inner class LibraryHandler is now static, so uniform access is possible from the unit tests, where no FatJarPackagerWizardPage is instantiated.
Nothing else changed.

Comment 63 Benno Baumgartner 2008-08-25 04:08:46 EDT

Thanks Ferenc! I've made the IP-Review request. 

https://dev.eclipse.org/ipzilla/show_bug.cgi?id=2597

This may take a while.

Comment 64 Ferenc Hechler 2008-09-16 19:56:26 EDT

Created attachment 112715 [details]
patch for org.eclipse.jdt.ui, add Jar-in-Jar ClassLoader (without zip-resource) 2008-09-16

Updated to HEAD.
Added code for Bug 243163: JarWriter4.addZipEntryStream also creates dir-entries.
Test: renamed JavaModelUtil.setCompilanceOptions to setComplianceOptions

The JUnit Tests are ok.
The GUI is untested, because I got Plugin-Conflicts.
The export-wizard does not appear in the List. 
In Fact only very few export wizards appear. 
Sorry, I will test the GUI soon.

Comment 65 Ferenc Hechler 2008-09-16 20:12:16 EDT

Fixed my workspace.
Tested the GUI for all three library-handling options.
Generated runnable JARs via GUI and via exported ANT-Scripts successfully.

No problems so far!   :)

Comment 66 Dani Megert 2008-09-17 03:51:10 EDT

Hi Ferenc, here's the comment from IP:

>Sorry for the late response.  Yes, there should be a similar dialog prompted to
>the end user for copying as well.  Go ahead and attach your proposal for that
>and we can take a look at it as part of the review.
>
>Also, can you please have Ferenc make the usual confirmations on the related
>bug and let me know when that is done.  Ferenc should be use to these by now:-)

>Confirmations:
>(a) wrote 100% of the code; 
>(b) that they have the right to contribute the code to Eclipse; and 
>(c) the file header contains the EPL License header


Can you attach the proposal for that dialog and answer the questions? Thanks.

Comment 67 Ferenc Hechler 2008-09-17 16:30:05 EDT

(In reply to comment #66)
> >Yes, there should be a similar dialog prompted to
> >the end user for copying as well.  

There are three options:

 ( ) Extract required libraries into generated JAR
 (o) Package required libraries into generated JAR
 ( ) Copy required libraries into a sub-folder next to the generated JAR

EXTRACT - the old behaviour.
PACKAGE - zip all libraries together into one jar. 
          The Libraries remain unchanged similair to WAR / EAR files 
          which also contain all libs inside.
COPY    - copy the libraries unchanged to the file-system

I do not think, that we need a dialog for COPY.

For EXTRACT the dialog says:

"This operation repacks referenced libraries.
Please review the licenses associated with libraries 
you wish to reference to make sure you are able to
repack them using this application. Note also that 
this operation does not copy signature files from
original libraries to the generated JAR file."

For PACKAGE the dialog could say:

"This operation packs referenced libraries together 
into one JAR file. Please review the licenses associated 
with libraries you wish to reference to make sure you 
are able to package them using this application."

> >Also, can you please have Ferenc make the usual confirmations 
> >on the related bug...

Of course:

I hereby confirm, that:

1. I authored 100% of the attached code for this bug
2. I have the rights to donate the code to Eclipse 
3. I am submitting the code for inclusion in future Eclipse releases under the EPL.

  feri

Comment 68 Dani Megert 2008-09-18 03:01:01 EDT

>I do not think, that we need a dialog for COPY.
I think so too but asked back in the CQ.

Comment 69 Chris Aniszczyk 2008-10-09 18:17:22 EDT

Cool work!

Comment 70 Dani Megert 2008-12-09 11:52:30 EST

We're almost done but still need to clarify some remaining legal issues.

Comment 71 Benno Baumgartner 2009-01-06 10:20:18 EST

Created attachment 121637 [details]
patch against HEAD

made FatJarBuilder abstract
disabled ip issue dialog for jar-in-jar and jar in folder exporter
some minor comment polishing

Comment 72 Benno Baumgartner 2009-01-06 10:32:59 EST

fixed > 01062009-0800

Sorry for the delay Ferenc and thank you very much for this contribution.

Comment 73 John Arthorne 2009-06-01 10:05:35 EDT

Removing iplog+ from bug - this indicates an IP contribution in a comment
rather than a patch.

http://wiki.eclipse.org/Development_Resources/Automatic_IP_Log

In this case since the contribution is covered by a CQ I think we don't need any flag in the bug. The CQ already appears in the JDT IP log.

Comment 74 Dani Megert 2009-06-01 10:15:40 EDT

Sorry John, but I disagree. I don't want to check n-tools in order to know whether something was contributed to JDT. The tool that compiles the log should be smart enough to catch the duplicates. If it is not capable of doing that then that's life and the readers of the generated IP log have to live with the duplicate entry.

Comment 75 John Arthorne 2009-06-01 10:33:37 EDT

That's fine. The main problem was that the flag was on the bug itself, so there were 50+ entries in the IP log for a single bug. Having the attachment on the single patch is fine and doesn't add much noise in the IP log.