Community
Participate
Working Groups
(there's no bugzilla version for 1.5.0M4) Release builds of aspectj have extra version checking that dev builds dont have, and this can cause problems in an AJDT environment. For example, I can't use AJDT with M3a to build AJDT with M4, because the installed aspectj M3a gets to the aspectjrt.jar in the workspace, and aborts because it's got a version of M4. Maybe the checking could be more sophisticated, or maybe it could be just a warning, and not abort compilation?
How does this happen?: "the installed aspectj M3a gets to the aspectjrt.jar in the workspace" I thought AJDT had an ASPECTJRT_LIB variable pointing to the correct version? This is related to Bug 112830. That suggests we check for aspectjrt*.jar variants, in part I think to avoid *not* seeing that the wrong version is first on the classpath and will be loaded. (e.g., "aspectjrt-1.0.jar:..:aspectjrt.jar"). The correct solution is to version the runtime and change it only when the runtime actually changes. Then each compiler needs to know its compatible runtime numbers. We've avoided that thus far. But that would only reduce the problem building AJDT to the times the runtime version was upgraded, so it seems AJDT should be able to specify the home aspectjrt.jar for a given compiler. Another is to support a flag permitting users to ignore the version check. If the runtime is n/a, they'll still get the "org.aspectj.lang.JoinPoint not found" error.
Just to clarify, AJDT uses the ASPECTJRT_LIB variable for regular AspectJ projects, but for AspectJ-enabled plugin projects, it instead needs to add a dependency on the org.aspectj.runtime plugin. Eclipse then resolves the classpath to use the workspace one (from M4), instead of the installed one (from M3a). This is a somewhat AJDT-specific scenario, but it seems to me that to abort because there's a *chance* of an incompatibility issue is quite drastic. Better would be more meaningful versioning as suggested, or at least make it a warning not an error.
This seems like a legacy test which has just bitten us again when using aspectjrt.jar (runtime) as a source plug-in dependency. It obviously has value for the commandline compiler where the user sets the CLASSPATH but seems to be redundant in an Eclipse environment which already performs adequate dependency and version checking. The test is fragile i.e. it relies on specific JAR names and perhaps a little expensive to be invoked for every compile. I think the test needs to be made more sophisticated or preferably ignored for Eclipse/OSGi (it can’t be that vital as we ignore it for DEVELOPMENT builds).
I think the fix is to remove the call to checkRtJar() from org.aspectj.ajde.internal.CompilerAdapter.compile(..). As the comment in that file suggests, the call is redundant. This was supposed to have been fixed when AjBuildManager.FAIL_IF_RUNTIME_NOT_FOUND was defined as false to cause these errors to be warnings. A better fix would be tied in with XLint to enable users to change the default value.
This aspectjrt.jar checking is at least partly responsible for bug 154483. Firstly, having all this logic only used in release builds is dangerous as it can lead to bugs not being discovered in dev builds. Secondly, as suggested above the whole check doesn't seem useful, at least in an Eclipse environments. I think either checkRtJar should be removed (note it's called from two places) or disabled for Eclipse, or at the very least made more consistent between release and dev builds.
checkRtJar removed - cant get confused any more.
The check prevents a whole lot of confusion (and support costs) for ant and command-line users, since it is fairly common to have more than one aspectjrt.jar in the system. It's a problem that is otherwise very hard to figure out, because the user will swear the right aspectjrt.jar is on the classpath, but the system is picking up the different one. I'd rather not have to answer these kinds of questions on the mailing list. The solution is to fix the opt-out designed for eclipse: change FAIL_IF_RUNTIME_NOT_FOUND to issue no message (not even a warning) when false and resolve the duplicate calls or guard both. I'd be happy to fix it, if you like.
I was just tired of getting it in the neck from users about this and so did something drastic and just got rid of it entirely since I have other problems to deal with - i was perhaps a bit hasty - I've not personally used this message to ever solve a problem I was having, but I'll admit I use AJ in perhaps an unorthodox way *8) Matt's point about consistency between dev and release builds is valid - I don't like their being a difference like this (as it only ever hits us with a problem when we've built the final release version of the product, and we have to respin...) and I think matthew will have some words to add. If you think you can fix it up properly Wes, based on further comments from Matthew, then please have a go.
Rather than trying to determine whether the user has got the CLASSPATH right we should follow the lead of AJDT and automatically add the right aspectjrt.jar. When I use javac I don't have to put the JDK on the CLASSPATH. When I use ajc I shouldn't have to set up the CLASSPATH for the compiler.
I'm not sure you *can* figure this out; you don't have the location of aspectjrt.jar when running in ant or launching via java -jar aspectjtools.jar, etc. Plus, the wrong-runtime scenario means that there are multiple runtime jars, and we'd have to not only find one, but find the right one.
>Plus, the wrong-runtime scenario means that there are multiple runtime >jars ... By this do you mean LTW? Also I assume the removed test was to ensure we have compatible versions of compiler > weaver > runtime rather than what the woven application will eventually run with. We have 3 scenarios where we need to get the right aspectjrt.jar 1. Batch compilation (commandline and Ant): The idea I had was for the launcher and Ant task to use "getClass().getProtectionDomain().getCodeSource().getLocation()" to find out where they were located then simply add the aspectjrt.jar that is co located to the classpath. This is essentially what AJDT does. 2. Eclipse/AJDT: AJDT puts aspectjrt.jar on the project classpath so no need for the check. 3. LTW: The calls to checkRtJar() are not on the code path for weaving and there is no CLASSPATH to check anyway. If we want to check versions in this environment we should do it my matching version classes (see Bug 69388 "Improve version identification").
In fact I think it is even simpler than that. Both the compiler and weaver _depend_ on the runtime and all the code is inside aspectjtools.jar anyway. The launcher and Ant task could simply add their own JAR to the build classpath.
I think we should behave as much like javac as we possibly can. In that respect aspectjrt.jar is effectively a bootclasspath entry (like rt.jar) and should be added accordingly but _only_ when the compiler is invoked from the command-line or Ant. However looking at the code for BuildArgParser.getClasspath() there is a reference to an old Bug 39959. However the report does not contain a satisfactory explanation for why removing aspectjtools.jar from the classpath was the solution and there is no testcase in the suite. Also I don't quite understand why we fall back on java.class.path if the user does not specify -classpath because this is _not_ consistent with javac. Is this to try and catch the CLASSPATH environment variable? In which case we should use System.getenv() instead.
Created attachment 49248 [details] Add aspectjrt.jar to bootclasspath PoC The few tests I have run, both unit and compiler, seem to pass. I have yet to persuade the harness to stop adding another version to the classpath e.g. fixupArgs() etc. When I have done that I can see if all tests pass with aspectjrt.jar automatically added.
In response to Matthew's request for comment... I've not commented on the comment 11 proposal because I haven't evaluated it yet. The suggestion assumes the user should be compiling against aspectjtools.jar, which to date we've never recommended. It also would put all the other classes in aspectjtools.jar on the classpath. I haven't reconsidered why we used to separate the runtime classes (to avoid misconfiguration) in light of the fact we don't now (not sure when that happened; we were careful not to before). Also I'm not sure the API is always available (e.g., in 1.3); otherwise, why didn't we do it before? So as a suggestion it raises a number of issues I haven't done the research to discuss. This bug has morphed somewhat into doing better version checking and classpath fixup. I don't think it is right to add *anything* to the bootclasspath or classpath that the user does not put there. Javac's use of rt.jar is a special case supported in the Java specifications. Users have gotten used to specifying aspectjrt.jar, and doing so reinforces that they have to run with the same jar. Basically, the user is responsible for ensuring that the same aspectjrt.jar is used at compile- and run-time. I'd prefer not to assume the classes in aspectjtools.jar are the same or assume that the aspectjtools.jar and aspectjrt.jar are co-located, etc. Right now the bug is assigned to Adrian. Andy's said I can investigate fixing the original implementation so it won't trip up AJDT. I'm happy to support proposals for better (automagic) functionality, but I'm extremely reluctant to do things that involve a policy or practice change for users unless it's a clear win because I think users should be able to upgrade versions without having to do (or worry about) anything more.
>The suggestion assumes the user should be compiling against >aspectjtools.jar, which to date we've never recommended. In the patch I have added the aspectjrt.jar that is co-located with aspectjtools.jar to the classpath instead to avoid any problems. >Also I'm not sure the API is >always available (e.g., in 1.3); It's an 1.2 API > ... otherwise, why didn't we do it before? I learn about APIs all the time and I've been using Java for 10 years! >I don't think it is right to add *anything* to the bootclasspath or >classpath that the user does not put there. I don't think bootclasspath has any special significance in ajc; it's just a question of ordering (see AjBuildConfig.getFullClasspath()) > Javac's use of rt.jar is a special >case supported in the Java specifications. and aspectjrt.jar is a special case for ajc (possibly) supported by the AspectJ specification >Users have gotten used to >specifying aspectjrt.jar, but that doesn't mean it's the right or best thing to do. >and doing so reinforces that they have to run with >the same jar. which is actually incorrect. In the general the environment that is used to build an application will be different to that which is used to run it e.g. Eclipse -> Ant -> Tomcat. We also support building an application with 1.5 and running on 1.2.1 as well as vice versa. In AJDT we automatically add aspectjrt.jar to the project classpath. We are more likely to get it right than the user. I think we should do the same for batch compilation.
AjBuildManager.java,1.115 has the check for correct aspectjrt.jar version re-enabled when run from Main.runMain(..) (taskdef, command-line) but still disabled otherwise (for AJDT) (yes, the duplicate call is still left out of CompilerAdapter). The static flag is unpretty but obvious. There's no test because it would involve mocking out static version text in Version.java, but I did that before checking in. I believe this bug is fixed for AJDT/Matthew's purposes. My objections to Matthew's proposed default still hold, and there should be a pretty big outcry from *users* before we make default user-visible changes, esp. in such a high-traffic area. However, it would be a great convenience to have a flag the user could use to request the compiler to seek out the (right) aspectjrt.jar. I'm leaving the bug open in case of outcry, but I'd rather this be closed and another opened (so long as it involves an explicit flag set by the user).
I'm presuming this is all done for 1.5.3? and closing - reopen if there is more to do.
