Bug 265103 - Manifest Class-Path is not read correctly with ECJ
Summary: Manifest Class-Path is not read correctly with ECJ
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.4.1   Edit
Hardware: All All
: P3 normal (vote)
Target Milestone: 3.5 M6   Edit
Assignee: Olivier Thomann CLA
QA Contact:
URL:
Whiteboard:
Keywords:
: 265105 (view as bug list)
Depends on:
Blocks:
 
Reported: 2009-02-17 04:09 EST by Fabrice Matrat CLA
Modified: 2009-03-10 11:40 EDT (History)
3 users (show)

See Also:


Attachments
Modified version of ClasspathJar.java and its unit test associated (3.87 KB, application/octet-stream)
2009-02-17 04:09 EST, Fabrice Matrat CLA
no flags Details
Proposed fix (11.27 KB, patch)
2009-03-02 12:48 EST, Olivier Thomann CLA
no flags Details | Diff
Proposed fix for HEAD (11.76 KB, patch)
2009-03-02 13:05 EST, Olivier Thomann CLA
no flags Details | Diff
Proposed fix (15.20 KB, patch)
2009-03-04 10:54 EST, Olivier Thomann CLA
no flags Details | Diff
Proposed fix for 3.4.x (16.23 KB, patch)
2009-03-04 11:11 EST, Olivier Thomann CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fabrice Matrat CLA 2009-02-17 04:09:38 EST
Created attachment 125864 [details]
Modified version of ClasspathJar.java and its unit test associated

The maximum length of a line in the Manifest is 72 characters. If a jar need to be split over two lines the next line should have a white space. If a jar finish at the end of a line the next line should start with two whitespaces.
If the next line start with a character it means, it's the next item and CLASS-Path is finished.

A full explanation of the way to read the Class-Path can be found for example at 
http://forums.sun.com/thread.jspa?threadID=364938

I attached a Unit test with some values and the fix I did in ClassPathJar.jar.
I already integrated the bug 251079 for the \r.

I don't know what we should do with an empty declaration of Class-Path.

See my changes attached and a unit test with it.
Comment 1 Fabrice Matrat CLA 2009-02-17 04:50:09 EST
*** Bug 265105 has been marked as a duplicate of this bug. ***
Comment 2 Olivier Thomann CLA 2009-02-23 11:47:29 EST
Two comments:
1) Why org.eclipse.jdt.internal.compiler.util.ManifestAnalyzer was moved as a static member class of org.eclipse.jdt.internal.compiler.batch.ClasspathJar?
It is also used in org.eclipse.jdt.internal.core.ClasspathEntry.
2) Why using the manifest analyser as a static field in ClasspathJar? This would not make it thread-safe.

Once I get answers to these concerns, I'll work on releasing the fix.
Comment 3 Fabrice Matrat CLA 2009-02-24 03:29:27 EST
1) Why org.eclipse.jdt.internal.compiler.util.ManifestAnalyzer was moved as a
static member class of org.eclipse.jdt.internal.compiler.batch.ClasspathJar?
It is also used in org.eclipse.jdt.internal.core.ClasspathEntry.

I took the code from 3.4.1 where the ManifestAnalyser was a static class inside ClassPathJar. I think it was moved in 3.5 beta version.

2) Why using the manifest analyser as a static field in ClasspathJar? This
would not make it thread-safe.

You are right but I didn't change that. The only changes I made where marked with // >>>>>>>>>>>>>>>>>> before the lines I changed.


We are using 3.4 here so that's why I used the code from 3.4 but apparently the manifest analyser class has moved to its own file in 3.5

Comment 4 Olivier Thomann CLA 2009-02-24 08:36:34 EST
Ok, I'll make the required changes to get a 3.4.x patch as well as 3.5 patch.
Thanks for your answers.
Comment 5 Olivier Thomann CLA 2009-03-02 12:34:34 EST
In 3.4 maintenance, the manifest analyzer is not longer a static field.
So I'll make appropriate changes to the proposed fix and will attach new patches for 3.4.x and HEAD for review.
Comment 6 Olivier Thomann CLA 2009-03-02 12:48:53 EST
Created attachment 127195 [details]
Proposed fix

Patch for 3.4.x including regression tests.
Comment 7 Olivier Thomann CLA 2009-03-02 13:05:59 EST
Created attachment 127196 [details]
Proposed fix for HEAD

Patch for HEAD including regression tests
Comment 8 Olivier Thomann CLA 2009-03-04 09:08:33 EST
Some of the org.eclipse.jdt.core.tests.compiler.regression.BatchCompilerTest tests are failing with this patch.
Needs more investigation.
Comment 9 Olivier Thomann CLA 2009-03-04 10:54:26 EST
Created attachment 127487 [details]
Proposed fix

New patch for HEAD that passes all batch compiler tests and the new test for the ManifestAnalyzer.
When the last line doesn't end with a line break, the corresponding jar entry should not be reported.

Fabrice, could you please review it?
Thanks.
Comment 10 Olivier Thomann CLA 2009-03-04 11:11:02 EST
Created attachment 127491 [details]
Proposed fix for 3.4.x

New fix for 3.4.x that is passing all BatchCompilerTests + new manifest analyzer tests.
Comment 11 Olivier Thomann CLA 2009-03-04 11:27:02 EST
Released for 3.5M6.
Released into 3.4.x branch (post 3.4.2)
Comment 12 Kent Johnson CLA 2009-03-10 11:40:43 EDT
Verified for 3.5M6 using I20090310-0100