Community
Participate
Working Groups
Build Identifier: 2.1.0.M04-incubation As discussed before we are trying to contribute the Inner Framework Detection commands into Virgo. I'll post in this enhancement request the intermediate patches until everything is fully ready to go productive and a final patch is posted. Below is the first intermediate patch. It includes enabling the inner framework commands with a startup parameter: example: >startup.bat -frk New components were created for the framework detection agent and lib, and the commands were included in the already existing osgishell Virgo component. These are covered with JUnits. One thing to note here is that the agent and the lib components are packaged without their versions - that is required since the -javaagent vm argument won't take '*' for the version of the jar, also in the manifest of the agent the both the detection and the javassist libs are set in the boot-classpath and again '*' are not permitted there, so the versions had to be removed. If that is a problem i'm open to any suggestions that can workaround that issue :) Also you'll notice there is also a javassist component created - thats until javassist goes further through the IP process so that it can be referenced with Ivy, when that point arrives i'll erase the component. It is here temporary for testing purposes only so that you can try how the commands are integrated within Virgo and return feedback. The thing that is missing for the moment are the integration tests. When they are completed i'll attach a new patch for review. Reproducible: Always Steps to Reproduce: 1.Apply patch & package Virgo 2.Uncomment the osgi.console property for the user region 3.Run Virgo with "startup.bat -frk" or "startup.sh -frk" 4.type "frk" in the OSGi console to check if the commands work
Created attachment 179096 [details] Intermediate innerFramework patch for testing purposes Feedback will be highly appreciated! :)
Created attachment 180637 [details] This patch contains the InnerFramework projects included in Virgo build with reference to javasisst library via Ivy Here is the patch with javassist referenced through Ivy as we talked about on the phone conference. I could not validate if it actually is buildable since I don't know with what org unit is the library included in the Ivy repositories. If it is known please post it here so i can resubmit a new patch or you can modify it when the new branch is created the dependency is described in the org.eclipse.virgo.kernel.frameworkdetection.javaagent's ivy.xml file. Currently it is set to "javassist.org". Could you also specify details on how i can connect and clone the newly created branch?
The new patch is incorrect as it includes some Java .class files. Did you use git diff to create it? I would hope git diff would ignore .class files unless they were accidentally added to version control. Also, for clarity, if some patches are superseded by later ones, please would you mark the old attachments as obsolete.
Thanks for the feedback, i'll generate it again. Just for information could you tell me what is the 'org' value in the dependency declaration for the javassist library, so that i can create the patch with the right dependency - currently i use "org.javassist".
(In reply to comment #4) > Thanks for the feedback, i'll generate it again. Thanks. > Just for information could you tell me what is the 'org' value in the > dependency declaration for the javassist library, so that i can create the > patch with the right dependency - currently i use "org.javassist". Based on the earlier versions, I think the org should be org.jboss.javassist.
For future reference, the SpringSource Enterprise Bundle Repository contains useful information about using bundles from Ivy and Maven. See for example: http://www.springsource.com/repository/app/bundle/version/detail?name=com.springsource.javassist&version=3.10.0.GA&searchType=bundlesByName&searchQuery=javassist.
Created attachment 180662 [details] Fixed patch I think this patch will be ok, i added the files one by one so if i didn't missed some files it should be good. I added the dependency to javassist as was specified in the EBR but still could not build it against the master. Anyway it should be ready for experiments with integrating it in a new branch. Again this only contains the build integration, not the runtime integration of the inner frk commands, I'll submit the latter when the new branch builds the current patch successfully. This way the changes will be incremental, which is good.
I can now see the wood for the trees in the latest patch. Is the Intermediate... patch now obsolete? As for the fixed patch, unfortunately it blows the 250 LOC limit by quite a margin so let me provide some feedback for you to incorporate before we create a CQ for it. We might at least reduce the amount of code that needs to be scanned. So the significant issues I have with fixed patch are: 1. There is much duplication of code, violating the DRY principle. The first example is the _frk method. Instead of a series of "if" statements, I think the code should be indexing into a map of command executors which are subclasses of a common interface. 2. Then there is much duplication in exception handling. In more than one case, this can be compressed by catching Exception and then calling a method to process the exception. The method can filter the exception type perhaps using a static set of exception types and re-throw if necessary. However, I'm also nervous about the amount of exception ignoring that is going on. Perhaps some comments would help? Example: + } catch (IllegalStateException ise) { + output.append("ERROR: Bundle installation failed:").append(NEW_LINE); + ise.printStackTrace(); + } catch (SecurityException se) { + output.append("ERROR: Bundle installation failed:").append(NEW_LINE); + se.printStackTrace(); + } catch (IllegalArgumentException e) { + output.append("ERROR: Bundle installation failed:").append(NEW_LINE); + e.printStackTrace(); + } catch (IllegalAccessException e) { + output.append("ERROR: Bundle installation failed:").append(NEW_LINE); + e.printStackTrace(); + } catch (InvocationTargetException e) { + output.append("ERROR: Bundle installation failed:").append(NEW_LINE); + e.printStackTrace(); + } catch (NoSuchMethodException e) { + output.append("ERROR: Bundle installation failed:").append(NEW_LINE); + e.printStackTrace(); + } 3. The FrameworkData class looks a bit "smelly". It appears to be little more than a holder of various fields. I wonder if there is behaviour in the users of the class which might move into the class to avoid having to expose so much of its state and thereby improve its encapsulation? I'll look at this again if nothing obvious presents itself. 4. m.setAccessible(true); makes me nervous. We should really be using the public facilities of OSGi frameworks rather than hidden methods which are likely to be unstable in the future. 5. I noticed the following which is confusing in an OSGi environment and will go out of date fast. + * @version 1.0 6. Finally all new files need suitable Eclipse license headers before we can submit them in a CQ. That's all for now, but probably enough to keep you busy for a while. Sorry if this is more exacting than you anticipated. :-)
The intermediate patch is obsolete, yes. I marked it as such when attaching the latest patch. I've updated the branch you created and i forked at github git://github.com/bkapukaranov/Virgo-kernel-sandbox.git with the improvements you requested and here are my comments on them: 1. code duplication - i introduced some kind of command executors and the code indeed looks cleaner now. Check them out to see if there is something wrong with them 2. duplication of exception handling - i unified it in a single method that handles those common exception types and returns a general message, i've also included '-debug' option for the subcommands so if the generic error message is of no help one could print the issue's stack trace and check out the root cause 3. The FrameworkData class is exactly a holder of various framework data fields. I introduced it because i needed an object that unifies all framework data that comes when a new framework is registered in the FrameworkCollector through the instrumentation mechanism. I think its fairly encapsulated - it has only one setter that could not cause much trouble. Maybe the logic that builds the framework IDs in ascending order could be moved up in the FrameworkCollector class so that FrameworkData will become fully encapsulated. I'll check that possibility later today because right now it seems it will be too disruptive instead of the current framework id build flow. 4. setAccessible(true) is required because i stumbled upon strange issues when using the commands on Felix nested frameworks - in older releases for some reason the OSGi interfaces were not correctly implemented or were just unfinished by that time, e.g. some methods that should be public were private and things like that. So to reassure that the required method is called i introduced that method - it currently provides compatibility with older Felix implementations. In general it should have no effect at all since all the methods that are invoked with reflection are public methods defined in the OSGi defined interfaces. 5 and 6. i removed the version header and updated the license headers. Check them out if there is something else that has to be added. Also i included that optional activation of the commands that we talked about - it is done with the '-frk' option. In general if you pull the changes and build the kernel and package it and then start Virgo with "startup -frk" the InnerFramework commands will be available for usage in the user region otherwise they would not be. Best Regards Borislav
I merged your changes into the latest master and a test failed: [exec] [junit] Testsuite: org.eclipse.virgo.kernel.osgicommand.internal.innerfrk.FrameworkInfoCommandPositiveTests [exec] [junit] Tests run: 11, Failures: 1, Errors: 0, Time elapsed: 0.062 sec [exec] [junit] [exec] [junit] ------------- Standard Output --------------- [exec] [junit] getprop [exec] [junit] Listing all properties for framework [38].. [exec] [junit] ---------------------------------------------------------- [exec] [junit] java.vendor = property_1 [exec] [junit] sun.java.launcher = property_2 [exec] [junit] sun.management.compiler = property_3 [exec] [junit] os.name = property_4 [exec] [junit] ant.file.org.eclipse.virgo.kernel.osgicommand = property_5 [exec] [junit] sun.boot.class.path = property_6 [exec] [junit] ant.file.common-standard = property_7 [exec] [junit] java.vm.specification.vendor = property_8 [exec] [junit] java.runtime.version = property_9 [exec] [junit] user.name = property_10 [exec] [junit] ant.file.package-common = property_11 [exec] [junit] ant.file.package-standard = property_12 [exec] [junit] awt.nativeDoubleBuffering = property_13 [exec] [junit] user.language = property_14 [exec] [junit] ant.file.default-standard = property_15 [exec] [junit] sun.boot.library.path = property_16 [exec] [junit] ant.file.publish-standard = property_17 [exec] [junit] ant.project.name = property_18 [exec] [junit] java.version = property_19 [exec] [junit] user.timezone = property_20 [exec] [junit] sun.arch.data.model = property_21 [exec] [junit] java.endorsed.dirs = property_22 [exec] [junit] sun.cpu.isalist = property_23 [exec] [junit] sun.jnu.encoding = property_24 [exec] [junit] file.encoding.pkg = property_25 [exec] [junit] file.separator = property_26 [exec] [junit] java.specification.name = property_27 [exec] [junit] java.class.version = property_28 [exec] [junit] user.country = property_29 [exec] [junit] java.home = property_30 [exec] [junit] java.vm.info = property_31 [exec] [junit] ant.file = property_32 [exec] [junit] os.version = property_33 [exec] [junit] path.separator = property_34 [exec] [junit] java.vm.version = property_35 [exec] [junit] java.awt.printerjob = property_36 [exec] [junit] sun.io.unicode.encoding = property_37 [exec] [junit] awt.toolkit = property_38 [exec] [junit] user.home = property_39 [exec] [junit] java.specification.vendor = property_40 [exec] [junit] java.library.path = property_41 [exec] [junit] java.vendor.url = property_42 [exec] [junit] ant.file.quality-standard = property_43 [exec] [junit] test-results.output.dir = property_44 [exec] [junit] java.vm.vendor = property_45 [exec] [junit] gopherProxySet = property_46 [exec] [junit] java.runtime.name = property_47 [exec] [junit] java.class.path = property_48 [exec] [junit] timestamp = property_49 [exec] [junit] java.vm.specification.name = property_50 [exec] [junit] ant.file.artifact-standard = property_51 [exec] [junit] java.vm.specification.version = property_52 [exec] [junit] sun.cpu.endian = property_53 [exec] [junit] sun.os.patch.level = property_54 [exec] [junit] java.io.tmpdir = property_55 [exec] [junit] java.vendor.url.bug = property_56 [exec] [junit] mrj.build = property_57 [exec] [junit] os.arch = property_58 [exec] [junit] java.awt.graphicsenv = property_59 [exec] [junit] java.ext.dirs = property_60 [exec] [junit] mrj.version = property_61 [exec] [junit] user.dir = property_62 [exec] [junit] ant.file.artifact-common = property_63 [exec] [junit] line.separator = property_64 [exec] [junit] java.vm.name = property_65 [exec] [junit] ant.file.publish-common = property_66 [exec] [junit] ci.build = property_67 [exec] [junit] file.encoding = property_68 [exec] [junit] ant.file.quality-common = property_69 [exec] [junit] java.specification.version = property_70 [exec] [junit] ant.file.common-common = property_71 [exec] [junit] [exec] [junit] ------------- ---------------- --------------- [exec] [junit] Testcase: test_frkVisibility(org.eclipse.virgo.kernel.osgicommand.internal.innerfrk.FrameworkInfoCommandPositiveTests): FAILED [exec] [junit] null [exec] [junit] junit.framework.AssertionFailedError: null [exec] [junit] at org.eclipse.virgo.kernel.osgicommand.internal.innerfrk.FrameworkInfoCommandPositiveTests.test_frkVisibility(FrameworkInfoCommandPositiveTests.java:340) I have pushed the branch to my sandbox so you can attempt to reproduce this failure. Presumably this has something to do with the merge of the changes from master. I admit I didn't run the tests in isolation before merging, but I can do that if you would like me to.
Note that the merged branch is sandbox/bkapukaranov-bug325560.
I ran the tests before pushing and they passed, i'll check if everything is pushed. I'm still kinda newbie with Git :) Thanks, I'll check it out.
I suspect the merge that I did is the root cause, either because of a functional clash or a bad merge. Please would you eyeball org.eclipse.virgo.kernel.osgicommand/template.mf to check it is correct as that was the one clash I had to edit manually.
The test failure in comment 10 was a testcase bug under UNIX. I have corrected this, worked around some build path issues, and fixed a couple of template.mf syntax errors. The full build now fails in an obscure manner during the first integration test ApplicationContextDependencyMonitorIntegrationTests which is in the ...kernel.test project. I observe that the MANIFEST.MF file of this project is radically modified by the build because the build places framework detection JARs in the project's src/test/resources folder. I suspect this is what is causing the test to fail, but that's only a theory. Since the build needs to be corrected to avoid updating src/test/resources, this should be done next to clear the way for debugging if still necessary. I have pushed my changes to my sandbox in github.
Has this been forgotten as I suspect that master is quite a long way off these patches now.
