eclipse-aspectj / ajdt

3 stars 6 forks source link

Remove AspectJ source and class files from AJDT repository #8

Closed kriegaex closed 2 years ago

kriegaex commented 2 years ago

Similar to how AspectJ now has a clean dependency on JDT Core instead of keeping a binary copy inside the SCM, AJDT should remove the copied class and source files from AspectJ, namely

and instead depend on the corresponding libraries, which are all public anyway. At the moment, Ant script org.aspectj.ajde/build-aj-jars.xml copies and unzips them, then they have to be committed to Git. The Ant script requires an AspectJ working directory and information about its location.

I have not analysed the build process any further yet, but I think the only thing that happens later is that the libraries are enriched with some OSGi meta information and then re-packaged into JAR archives again, only to be included in the AJDT installation package. I think that it should be possible to generate this meta information and add it to the existing JAR files on the fly using some appropriate Maven plugin.

It might also be worth investigating if it would somehow be possible not to include the libraries into AJDT but instead reference and dynamically download them. Maybe we can even add a settings dialogue in order to enable independent AspectJ version upgrades or project-specific switches without necessarily having to release a new AJDT version. That would save us cycles and make users more flexible. @aclement, WDYT?

@aeisenberg, I would like to hear your opinion about the last idea as well.


I used a tool called Grand in order to graphically represent the Ant targets from build-aj-jars.xml:

image

Other than the main goal plugin jars which copies the classes and sources from the 3 main AspectJ artifacts, the AJDT build does not use the other Ant targets anymore, because we build with Maven. I just wanted to document it a little bit, before I remove than Ant script in favour of a pure Maven solution.

aclement commented 2 years ago

The embedding of sources/classes in that way should have been addressed long ago.

aeisenberg commented 2 years ago

I think this makes sense to avoid repackaging and renaming an existing JDT.

Dynamic download will be a lot of work, but a nice user experience if you can get it working. You would need to be very careful about declaring which versions of JDT are compatible. I'd expect that the range would be very small. Small enough that it's not much more flexible than hard coding a single version.

kriegaex commented 2 years ago

Dynamic download will be a lot of work, but a nice user experience if you can get it working.

A lot of work sounds like something I do not feel inclined to do right now, given the fact that my interest in learning Eclipse OSGi and especially Eclipse GUI is somewhat limited, to say it euphemistically.

You would need to be very careful about declaring which versions of JDT are compatible. I'd expect that the range would be very small. Small enough that it's not much more flexible than hard coding a single version.

Last night I also thought about that, especially because since I was involved in "maintaining" AJDT (i.e. keeping it alive somehow) I did not care about backward compatibility at all, already being glad to get it working again after merging in new AspectJ and JDT stuff.

So I think that I am going to limit the implementation to getting rid of the AJ libraries in the AJDT Git repository, and the extra ribbons and laces are off the table for now. Thanks to both of you for your feedback.

kriegaex commented 2 years ago

@aeisenberg, @aclement, when I started looking into this, I noticed that

Currently, the filtering is done by the Ant script copying the content from a local AspectJ working directory.

I do not know what AJDT puts on the classpath when running applications, but like the JARs are packaged now, I would assume that

This is somewhat irritating, because AJDT it does things differently from how a normal user would do it when using command line or Maven. But OK, it works and avoids redundant classes in the download packages. Am I understanding the situation correctly?

https://github.com/eclipse/ajdt/blob/9f7a0fee27b49ae845019ad5f993e47325c14c5d/org.aspectj.ajde/build-aj-jars.xml#L113-L125

Edit: Hm, it seems that, despite the XML comment "delete everything which is in ajde", in fact not everything is excluded, but ajde/core is retained.

Edit 2: Answering my own question: Yes, AJDT does in fact import org.aspectj.ajde.core classes in some places, so it was not an oversight to retain them. I will therefore do the same when migrating this from Ant to Maven and from SCM storage to dependency download & unpacking.

kriegaex commented 2 years ago

One more question:

Edit: Actually, the manifest is completely re-written for the weaver, because AJDT contains a redundant OSGi version of the weaver manifest in org.aspectj.weaver/META-INF/MANIFEST.MF. Therefore, retaining it seems unnecessary.

kriegaex commented 2 years ago

Next question: For now I am keeping the source and classes directories directly in the corresponding module folders, because there are Eclipse .classpath files referring to them as library paths, e.g.: https://github.com/eclipse/ajdt/blob/f90b253357693a0a75a1c5a0d2a1133f52ab2243/org.aspectj.ajde/.classpath#L4 But actually, I would be happier to move those paths to the Maven target folder where they would be cleaned up by Maven automatically without an extra Maven Clean configuration and where they would not need to be ignored by extra .gitignore entries, which I am going to introduce in my working PR.

kriegaex commented 2 years ago

@aclement, I am still waiting for answers to the questions marked by unchecked check boxes, even though the issue is closed after having merged I the simple solution (without moving source and binary directories to target, instead adding them to .gitignore) a few days ago.