Community
Participate
Working Groups
I20130502-0800 org.eclipse.osgi.services.source_3.3.100.v20130327-1440.jar and org.eclipse.osgi.util.source_3.2.300.v20130327-1440.jar have various problems: - contain 2 copies of *.class files (at root and inside 'target' folder) - contain other wrong content like .classpath and .project - the source files/folders are not at the root of the JAR but in an embedded src.zip I didn't see such problems in other bundles.
This project has a PDE build customBuildCallbacks script that obviously is not getting used during the CBI builds. At this point I am unsure how we fix this in CBI.
Created attachment 230476 [details] services.patch Had some free time this morning so I thought I'd give this a try. The attached patch produces a sources.jar with the java sources in root. However there are several things wrong with this patch since I made some assumptions that are likely wrong as well as caused other issues. Assumptions: - src.zip is pre-unzipped into the bundle directory - tried to avoid using maven-antrun-plugin - that it was ok for *.class and *.java files to exist in the same directory Issues: - the non-source.jar now contains *.java files in addition to *.class files :( - you must manually unzip src.zip for the bundle before running the build I tried putting src.zip files into a subdirectory such as src/ but when I added that into "src.includes = src/" it would include the src/ folder as part of the path too, I'm not sure if there's a way to tell build.properties to use src/ as a root path? I'm probably taking the wrong approach but hope it spurs some ideas.
Should try to get this fixed for 4.3.
(In reply to comment #2) > - tried to avoid using maven-antrun-plugin I think at this point in Kepler, if we can fix using antrun plugin, using existing "customBuildCallbacks", that would be suffice (i.e. great even). No idea if the customBuildCallback would work "as is", or need a lot of adjustment ... just saying we should not hesitate to use antrun at this late date.
Created attachment 230622 [details] patch v2 I think I have a working patch now. I use antrun to unzip src.zip into the project dir so now both *.java and *.classes files exist in the same "org/" directory. I then use tycho-packaging-plugin's "additionalFileSets" configuration to filter only *.classes files for inclusion in the bundle.jar. I also removed "org/" from build.properties bin.includes= parameter so that it doesn't pull in the *.java files. I use tycho-source-plugin to filter "*.java" and "packageinfo" files for inclusion in the sources.jar As far as I can tell the bundle.jar and sources.jar appear to create similar contents from those of Juno SR2 PDE build. One discrepancy I noticed but I'm not sure if it's a problem is there appears to be adding additional metadata files in the sources.jar listed below. creating: OSGI-INF/ creating: OSGI-INF/l10n/ inflating: OSGI-INF/l10n/bundle-src.properties
I will review and release patch.(In reply to comment #5) > As far as I can tell the bundle.jar and sources.jar appear to create similar > contents from those of Juno SR2 PDE build. One discrepancy I noticed but I'm > not sure if it's a problem is there appears to be adding additional metadata > files in the sources.jar listed below. > > creating: OSGI-INF/ > creating: OSGI-INF/l10n/ > inflating: OSGI-INF/l10n/bundle-src.properties That is strange, I will have to take a look at the content when a build is done. I am curious if the source bundle has a Bundle-Localization header.
An issue with the patch is that it breaks exporting the bundle using PDE from the workspace. Not sure if we can get a solution that works for both CBI and PDE-Build. If I had to choose I would get the CBI build correct since breaking the export of this project from the workspace will impact very few developers. We can always add a PDE specific build.properties that one would need to rename when trying to export these bundles if they need to.
I went ahead and released this fix so that we can get a test build: http://git.eclipse.org/c/equinox/rt.equinox.framework.git/commit/?id=6447261180fce2fef30fbdc7175b49fa025f6864
(In reply to comment #8) > I went ahead and released this fix so that we can get a test build: > > http://git.eclipse.org/c/equinox/rt.equinox.framework.git/commit/ > ?id=6447261180fce2fef30fbdc7175b49fa025f6864 Well, it didn't break the build. But, no idea if its right. :) You can "see" the results of test build on build machine at http://build.eclipse.org/eclipse/builds/4I/siteDir/eclipse/downloads/drops4/I20130508-1535/
The content of the source bundles looks correct to me, but they still don't seem to work as proper source bundles by PDE. I'm not sure why but when looking up any source contained in the osgi.services bundle PDE is not finding the source still.
To cross reference, think these changes are related to bug 407623. FYI, there is long bug where Curtis was explaining what form source had to be in for multiple source folders ... bug 400740 ... perhaps that'd give some insight?
Curtis, could you have a look at the latest org.eclipse.osgi.services.source bundle to see if you can spot what is wrong with it from the PDE perspective?
(In reply to comment #12) > Curtis, could you have a look at the latest org.eclipse.osgi.services.source > bundle to see if you can spot what is wrong with it from the PDE perspective? I haven't debugged, but the source roots is listed as "", when it should be ".".
The missing '.' (meaning the source is at the root of the bundle) is the problem, but not because PDE is looking at the source in the wrong place. We actually get an exception when trying to parse the header. It appears that having a header value of a 0 length string gets replaced with null and osgi utils throws an exception. PDE throws the source bundle out as its header can't be parsed. org.osgi.framework.BundleException: Invalid manifest header Eclipse-SourceBundle: "org.eclipse.osgi.services;version="3.3.100.v20130508-1933";roots:=""" at org.eclipse.osgi.util.ManifestElement.parseHeader(ManifestElement.java:414) at org.eclipse.pde.internal.core.BundleManifestSourceLocationManager.setPlugins(BundleManifestSourceLocationManager.java:202)
I think this is because my patch removes the line: sources..=. But if I don't remove that line Tycho will try to compile the code and fail.
(In reply to comment #14) > The missing '.' (meaning the source is at the root of the bundle) is the > problem, but not because PDE is looking at the source in the wrong place. > We actually get an exception when trying to parse the header. It appears > that having a header value of a 0 length string gets replaced with null and > osgi utils throws an exception. PDE throws the source bundle out as its > header can't be parsed. > > org.osgi.framework.BundleException: Invalid manifest header > Eclipse-SourceBundle: > "org.eclipse.osgi.services;version="3.3.100.v20130508-1933";roots:=""" > at > org.eclipse.osgi.util.ManifestElement.parseHeader(ManifestElement.java:414) > at > org.eclipse.pde.internal.core.BundleManifestSourceLocationManager. > setPlugins(BundleManifestSourceLocationManager.java:202) I could look into allowing empty string directives. I need to look at the OSGi spec details to see if that should be allowed, if so then the BundleException could be considered a bug in the framework manifest parser. But if I did that would PDE treat the empty string as an alias for '.'?
(In reply to comment #16) > But if I did that would PDE treat the empty string as an alias for '.'? I don't think this is a good idea.
(In reply to comment #17) > (In reply to comment #16) > > But if I did that would PDE treat the empty string as an alias for '.'? > > I don't think this is a good idea. What else would empty string mean? Just to confirm, it does seem to be a bug in the framework manifest parser. The syntax for quoted strings is: quoted-string::= ’"’ ( ~["\#x0D#x0A#x00] | ’\"’|’\\’)* ’"’ So my question still remains. If we fixed that what would PDE do?
(In reply to comment #18) > (In reply to comment #17) > > (In reply to comment #16) > > > But if I did that would PDE treat the empty string as an alias for '.'? > > > > I don't think this is a good idea. > > What else would empty string mean? It could e.g. mean "not having source". Implicit assumptions are a maintenance hell.
(In reply to comment #19) > (In reply to comment #18) > > (In reply to comment #17) > > > (In reply to comment #16) > > > > But if I did that would PDE treat the empty string as an alias for '.'? > > > > > > I don't think this is a good idea. > > > > What else would empty string mean? > > It could e.g. mean "not having source". Implicit assumptions are a > maintenance hell. I understand your point about assumptions, but I don't understand why a bundle would have Eclipse-SourceBundle and "not having source". Also, I am wondering what would happen if there was not roots directive at all for this header? is there some default value for the roots directive?
The docs state: If no roots directive is specified, then a value of "." is assumed.
(In reply to comment #21) > The docs state: > > If no roots directive is specified, then a value of "." is assumed. Point taken! ;-).
*** Bug 407868 has been marked as a duplicate of this bug. ***
Created attachment 230863 [details] patch fix missing root If it matters, attached patch fixes the issue so that the bundles are now setting root="." again by restoring lines: source.. = . output.. = . I was able to workaround tycho-compiler-plugin trying to compile the sources by disabling the plugin for these 2 bundles by setting the build phase to "none".
(In reply to comment #24) > Created attachment 230863 [details] > patch fix missing root > > If it matters, attached patch fixes the issue so that the bundles are now > setting root="." again by restoring lines: > > source.. = . > output.. = . > > I was able to workaround tycho-compiler-plugin trying to compile the sources > by disabling the plugin for these 2 bundles by setting the build phase to > "none". Thanks Thanh, at this point I think this is the more safe option rather than fixing the manifest parser and hoping PDE 'does the right thing'. FWIW in my branch for luna I have already fixed the manifest parser. I discovered this while implementing some new OSGi R6 native environment requirements. I knew this sounded familiar.
I released the latest patch from Thanh with: http://git.eclipse.org/c/equinox/rt.equinox.framework.git/commit/?id=8f4dc6841991e7a9977e40c8bad5dd8bc39b641e
Created attachment 230892 [details] Summary of compile errors in test build Any chance this latest change would cause compile errors like those attached? I was trying a test build (for other reasons) and got these errors. Of course, is could be something completely unrelated (perhaps related to "the test" I was trying out) .... but, thought I'd report it as soon as I could, in case related to this change.
(In reply to comment #27) > Created attachment 230892 [details] > Summary of compile errors in test build > > Any chance this latest change would cause compile errors like those > attached? > I would say this is highly likely. I am going to revert the change and you can test it out. > I was trying a test build (for other reasons) and got these errors. Of > course, is could be something completely unrelated (perhaps related to "the > test" I was trying out) .... but, thought I'd report it as soon as I could, > in case related to this change. I think it is pretty safe to say this fix has something to do with this. The runway is getting short and I think any changes here are going to continue to cause us grief while we try to stabilize and ramp-down our build. I have reverted both commits until we can get a complete and well tested solution. Thanks Thanh for all your help! If you have something else you would like us to try please let us know, but at this point I am leaning to deferring this fix until SR1. Reverted commits: http://git.eclipse.org/c/equinox/rt.equinox.framework.git/commit/?id=1b5390f6a774b6098d09c7db041d8885f5683dce http://git.eclipse.org/c/equinox/rt.equinox.framework.git/commit/?id=4a43819806e41156205f1bea2bd52011f409f242
(In reply to comment #28) > (In reply to comment #27) > > Created attachment 230892 [details] > > Summary of compile errors in test build > > > > Any chance this latest change would cause compile errors like those > > attached? > > > > I would say this is highly likely. I am going to revert the change and you > can test it out. > No compile errors in my local test build (which is faster than build.eclipse.org, since not signed ... one on build.eclipse.org still running).
I added bug 407623 to the "blocks" list, since I think "fixing" this bug caused that bug ... so, by implication, any proposed fixes should be sure to be tested with "the comparator" to be sure these stray classes are not sprinkled around. If I had to take a wild guess, I'd guess the was some "weaving" or "injector" type jar in the "source jars" that some how gets "on the classpath" and has some effect on subsequent build steps. Probably using with wrong terminology, but hopefully the concept is clear (wild guess as it is). In any case, I'd suggest deferring also, unless a simple, safe fix is imminent. One that tests not only that "the right source jars are created", but that it does not have any side effects.
Tentatively setting to SR1. Probably a good question to investigate, though, is if these bundles had "source bundles" in Juno. I seem to recall there were always some missing. Not sure which, thought. (That is, if regression, probably should be addressed in SR1 ... but if not a regression, I'd suggest Luna).
(In reply to comment #31) > Tentatively setting to SR1. > > Probably a good question to investigate, though, is if these bundles had > "source bundles" in Juno. I seem to recall there were always some missing. > Not sure which, thought. (That is, if regression, probably should be > addressed in SR1 ... but if not a regression, I'd suggest Luna). Yes, these bundles had source bundles in Juno. This is a regression.
For luna I plan to build the osgi source properly (see bug 411404). This is not an acceptable solution for Kepler SR1 though since it will involve removing an API package org.osgi.service.io from org.eclips.osgi.services (although it is likely a package that nobody will notice is missing).
Assuming no changes planned here for Kepler? Let me know if I've misunderstood.
(In reply to comment #34) > Assuming no changes planned here for Kepler? Let me know if I've > misunderstood. No, but this should be fixed in Luna since we now build these projects like normal projects.
This seems fixed in Luna. I'm going to arbitrarily put "M5", but Tom, please re-open if there's something more to do, or set target accurately if you now.
