-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Upgrade to ASM 4.0 #5
Comments
In branch topic/asm4 I migrated JaCoCo to ASM 4.0. ASM 4.0 is available as OSGi bundle in the Maven repo (org.objectweb.asm.all). I tried to make it part of our target patform with pomDependencies=consider but did not succeed so far. |
According to http://search.maven.org/#search, there is
with
But org.jacoco.build/pom.xml and org.jacoco.core/pom.xml still declare dependency on
Is it OK? |
I know the artifact is there and I also tried to adjust the build (with the correct version number of course). But I did not succeed make Tycho using this reference part of the target platform. Maybe I missed something obvious. I you have a running version of the build I would be happy if you could share the patch! |
Hi guys, here is a story - dependency, which declared in pom.xml, used only for consumers of JaCoCo (i.e. other projects which depends on it), and resolution of OSGI bundles happens from p2 repositories instead of Maven. And ASM still not available in p2 repositories. Moreover - I suppose that we will not be able to release EclEmma with ASM 4 until resolution of https://bugs.eclipse.org/bugs/show_bug.cgi?id=380801 |
For the technical part at least documentation says that we should be able to pull the OSGi bundle from Mave repo: http://wiki.eclipse.org/Tycho/Target_Platform#.22POM_dependencies_consider.22 But I didn't manage to get this work. |
It is not possible to mix pom-first and manifest-first projects in the same reactor build - http://wiki.eclipse.org/Tycho/How_Tos/Dependency_on_pom-first_artifacts#It_is_not_possible_to_mix_pom-first_and_manifest-first_projects_in_the_same_reactor_build. |
From my understanding, build should be performed in two steps:
bundle?! May be it's a silly Q but: why org.jacoco.core module has an eclipse-plugin packaging? As I see, it's a plain JAR with OSGi manifest without any dependencies on Eclipse stuff. Am I missing smth? |
It works without changing any packaging types, against an empty local repository, using Apache Maven 3.0.4, but I still have to clean up history a little:
|
Correct, JaCoCo has no dependencies on Eclipse. But JaCoCo needs to ship as proper Eclipse bundles. For this and for consistency with dependency management in the IDE we use Maven/Tycho. |
But you could change core's packaging to jar and use maven-bnd-plugin for making it an OSGi bundle. It would be almost the same, I hope. But anyway, my changes are available here https://github.com/manandbytes/jacoco/tree/topic/asm4, feel free to comment. |
@marchof In fact now I'm not sure that we should stay with Tycho. We're stuck with a current version - build will not work with newer versions of Tycho. I need time to explain this problem more detailed. Moreover Tycho is too slow (at least version which we use). And as correctly was pointed by @manandbytes we could imagine to use maven-bnd-plugin to create OSGi bundles. Dependency management in IDE already not a problem, because of M2E. And to verify OSGi bundles we have EclEmma. By dropping Tycho we also could simplify build. But again - I need some time to describe pros and cons... |
Merged to master with commit dd44098 |
Godin
Seems that release of ASM 4.0 planned on October 29, so we can upgrade JaCoCo in order to support new Java 7 instruction invokeDynamic.
marchof
This has a implication on EclEmma: Currently EclEmma ships without ASM, as ASM 3.x is part of Eclipse. If we migrate to 4.0 we need to include it with EclEmma.
Another thing is an license issue: We should wait until ASM 4.0 is IP approved by Eclipse, otherwise we lock JaCoCo out from official usage within Eclipse (e.g. JaCoCo is currently used for platform build).
Changed 9 months ago by mtnminds
Actually as a first step I would like to upgrade to 3.3.1. As all tests passes and this version has lots of bug fixes I will upgrade the dependency to 3.3.1 for now.
centic@SF
FYI, EclEmma 2.0.1 will ships with the ASM-bundle included because some Eclipse installations do not have it (i.e. if PDE is not installed), thus the first item should be covered starting with this release.
marchof
In the meantime I checked with Eclipse: They don't see any issues with ASM 4.0 IP approval, so we can go for it!
marchof
Another open issue: How do we consume ASM 4.0 as a bundle? It's not yet available from eclipse.org. For the EclEmma distribution we need a signed version of this bundle.
Changed 3 months ago by mtnminds
Opened bug against orbit to provide ASM 4.0:
https://bugs.eclipse.org/bugs/show_bug.cgi?id=380801
marchof
Today I used some travel time for a prototype of JaCoCo based on ASM 4.0. There were two main issues:
ASM ClassVisitor and MethodVisitor has been converted to abstract classes. This breaks some internal interfaces in JaCoCo's flow analysis package which used to extend the ASM interfaces. For now I simply converted them also to classes which results in ugly class hierarchies. But this can be certainly refactored to a better structure.
The other issue was more tricky as it is about ASM runtime behavior only. Thanks to our regression tests I was quickly aware that the control flow analysis was completely broken at the beginning. The tree API of ASM 4.0 now replaces Label instances after every MethodNode.accept() call. This seriously hurts JaCoCo as we attach control flow information to label instances. My current workaround is to manually loop over the instruction list. But this hack might be better encapsulated when the internal APIs are redesigned anyways due to the issue above.
The text was updated successfully, but these errors were encountered: