WALA icon indicating copy to clipboard operation
WALA copied to clipboard

Proposal: move com.ibm.wala.ide.* projects to a separate Git repository

Open msridhar opened this issue 6 years ago • 12 comments

I propose we move the com.ibm.wala.ide.* projects out of the wala/WALA repo and into a sibling repo (similar to wala/ML). Pros:

  • Will speed up Gradle builds of wala/WALA, as we'll no longer need the p2AsMaven integration. (Caching makes this mostly a one-time cost, but there are various scenarios where the download needs to be repeated.) (Update: p2AsMaven was removed in #587 so this pro is now moot.)
  • We can deprecate building this repo with Maven, which will speed up CI significantly (the Maven Travis jobs take about twice as long as the Gradle ones). We will also get away from having a duplicated build structure that needs to be maintained.

Cons:

  • We can asynchronously detect if/when a core WALA change breaks the IDE functionality, but we won't be blocked from landing such changes.
  • Various ECJ-based checks only run on the Maven builds now. Without an alternative, we would no longer block the build on those detected issues (e.g., unused imports).

I would argue that few WALA users rely on the IDE functionality, particularly since there is an ECJ-based Java source frontend now. So I think this is the right tradeoff. For the second con, I would argue we should move to Error Prone, which integrates through standard Java compiler interfaces and also has many of the checks built into ECJ. Plus it integrates easily with Gradle.

@liblit @juliandolby thoughts?

msridhar avatar May 04 '19 18:05 msridhar

I am generally in favor of anything that lets us retire the Maven build system. I would also be happy to see the slow p2AsMaven downloads go away. I think that those two pros outweigh the two cons, so overall I support this proposal.

liblit avatar May 05 '19 02:05 liblit

I see several Gradle plugins that switch Gradle to use the ECJ compiler for batch compilation. If we can make the choice of compilers selectable on the ./gradlew command line, then we can have standard-compiler Travis CI jobs and also ECJ Travis CI jobs.

If we can make that work cleanly, I think that would be a good idea even if we do get Error Prone wired up. Error Prone will never be identical to what Eclipse will find. If we want to keep the code ECJ-clean, then we should include ECJ builds in our testing regime.

liblit avatar May 05 '19 03:05 liblit

I am mostly in favor of this as well, but I wonder about a few things:

  1. We definitely need to keep the ECJ-based Java source front end working with the latest WALA, as that is being actively used in current projects. I think we need to block changes that break it.

  2. I think currently all tests of the ECJ-based source front end are actually in the com.ibm.wala.ide.jdt.test project, so we'd likely need a new test project in the core for that code.

  3. I am less concerned about the Eclipse IDE integration, but we should have a mechanism that to ensure that breaking changes get noticed promptly and fixed.

juliandolby avatar May 15 '19 17:05 juliandolby

  1. We definitely need to keep the ECJ-based Java source front end working with the latest WALA, as that is being actively used in current projects. I think we need to block changes that break it.

Absolutely. As far as I know the ECJ front-end and Gradle play together just fine.

  1. I think currently all tests of the ECJ-based source front end are actually in the com.ibm.wala.ide.jdt.test project, so we'd likely need a new test project in the core for that code.

Cool, good to know and should be doable.

  1. I am less concerned about the Eclipse IDE integration, but we should have a mechanism that to ensure that breaking changes get noticed promptly and fixed.

So here we have two options:

  1. Trigger the tests for the new Eclipse IDE project every time a commit lands on WALA master, just like we do for wala/ML, etc. This triggers immediate failures for a breaking change, but requires someone to be actively monitoring the sub-project to notice. It's not clear to me that any of the main WALA developers cares enough to do this.
  2. Make the new Eclipse IDE project depend on a WALA release version. This way, whatever happens on master, the Eclipse IDE code stays working. If/when someone actually needs a new feature from WALA master for the Eclipse IDE code, we would explicitly cut a release, and then go update the Eclipse IDE code as needed to work with that release. (We should be cutting releases more frequently anyway.)

Since the WALA Java and JS frontends are fairly stable, and the Eclipse IDE code doesn't get much use (not even clear to me it works on more recent Eclipse versions), I think 2 might be a better option. Plus, it will be a better experience for anyone using the Eclipse IDE code, as they will not have to build core WALA from source.

My proposal is to go ahead with the split, using approach 2 of the new project depending on the latest WALA release. Objections? In the meantime I'll go ahead and start the work.

msridhar avatar May 15 '19 17:05 msridhar

I am almost done with the work to complete this task locally. However, having done the work, I now wonder whether a different solution might be better. Rather than moving the com.ibm.wala.ide.* out of the main WALA repo, we could instead change things so that only those projects are built and tested with Maven. Rather than treating the other WALA projects as Maven sub-modules, we would install their artifacts to the local .m2 repository, and then use SNAPSHOT dependencies from com.ibm.wala.ide.* to consume them. I already basically have this working locally.

Advantages of keeping com.ibm.wala.ide.* in the main repo:

  • Breakage gets noticed and fixed immediately. In particular, we can keep the extant Gradle build files in com.ibm.wala.ide.* so at least compile errors are noticed on any local Gradle build.
  • We won't need to upload artifacts for our CAst Java test fixture code to Maven Central, which is kind of weird and almost entirely useless outside of these projects.
  • We can still remove most of the Maven pom.xml files, Ant build.xml files, and related Eclipse plug-in build files; such files only need to remain in the IDE projects (and I don't think there are any build.xml files in those projects, so Ant would be gone).

Disadvantages:

  • We still need to run a Maven build on CI. But this can run much faster than before, since there are many fewer projects to build. We could tack on the Maven run to an extant Gradle build. Or, we could keep the Maven build and add ./gradlew install to the preparatory steps to get the artifacts installed. (For a further speedup, we could disable generation of Javadoc and source jars in ./gradlew install on that build, if there's a way to do that.)
  • You still won't see all potential test failures on local builds unless you run both Gradle and Maven. But, assuming we want to keep com.ibm.wala.ide.* working, moving that code out of the main repo just makes that problem worse.
  • The main repo documentation will be "cluttered" with any explanations of how to use and test com.ibm.wala.ide.*. But I think the overall story will be mostly simple. If you don't care about com.ibm.wala.ide.*, you can just use Gradle and everything should work.

Given the above, I am leaning toward keeping com.ibm.wala.ide.* in the main repo and "shrinking" the Maven build to only cover those projects.

@liblit @juliandolby thoughts?

msridhar avatar Dec 15 '19 15:12 msridhar

My WIP is here:

https://github.com/msridhar/WALA/tree/shrink-maven

Getting things to work in-repo is basically a pre-requisite for moving the projects out of the repo, so I'm going to continue working on that first. I think it's a good intermediate step to aim for.

I also realized another small advantage of keeping things in repo, which is that we'll have a test that our process of cutting artifacts is working correctly (since the IDE projects will depend on core WALA projects mostly via artifacts).

I'm currently trying to figure out what to do with com.ibm.wala.cast.java.test.data. I realized there were undeclared dependencies that at least partly explain why our current Maven builds are so weird (you need to run verify -DskipTests once to get verify to pass without skipping tests); sigh.

msridhar avatar Dec 16 '19 01:12 msridhar

The original motivations for separating com.ibm.wala.ide.* from the main WALA repository were to get rid of the slow p2asmaven step and to deprecate Maven as a build engine. The first of these two motivations is now moot (#587). I had still hoped to see Maven go away completely, but @msridhar's latest proposal seems to mean that we would be stuck with Maven forever. Is it just impossible to build and publish com.ibm.wala.ide.* using Gradle?

liblit avatar Dec 16 '19 01:12 liblit

Compiling these projects already works with Gradle. The key problem is that the tests in ide.jdt.test and ide.jsdt.test are Eclipse plug-in tests, which need to run inside a running Eclipse instance. The only way I know how to get that working in an automated build is with Tycho, which only works with Maven. I don't know of any Gradle equivalent for Tycho, and some Google searching didn't turn up anything.

The real question here is do we need to keep these Eclipse plug-in tests working. If the answer is yes, then given what we know now, someone is going to be stuck with Maven, whether it's in the main WALA repo or somewhere else.

msridhar avatar Dec 16 '19 01:12 msridhar

Glad we documented all this stuff before! When I get time I will look into creating the separate Git repo. Dealing with tests will be a bit tricky due to the dependence on com.ibm.wala.cast.java.test.data. My best idea for now is that the new Git repository will rely on checking out the WALA git repo (at a particular release tag) and running the same ./gradlew command we run now to get that project into shape. But I'll think more.

msridhar avatar Sep 10 '21 17:09 msridhar

Dealing with tests will be a bit tricky due to the dependence on com.ibm.wala.cast.java.test.data.

Is the challenge here that com.ibm.wala.cast.java.test.data is not currently available from Maven Central as a published build artifact? If that’s what makes things tricky, then I’d naïvely say that one of two things should be true. Either

  1. depending on com.ibm.wala.cast.java.test.data from outside of the main WALA packages is OK, and therefore com.ibm.wala.cast.java.test.data should publish a reusable artifact; or

  2. depending on com.ibm.wala.cast.java.test.data from outside of the main WALA packages is not OK, in which case the new repository’s tests need to change so that they don’t use this material.

liblit avatar Sep 11 '21 01:09 liblit

So here is what makes things tricky. First, the tests in both com.ibm.wala.cast.java.ecj and com.ibm.wala.ide.jdt.test depend on the testFixtures configuration of com.ibm.wala.cast.java. I think we could turn this into a Maven artifact without too much trouble, so that com.ibm.wala.ide.jdt.test can still use it after being moved out.

Both projects also read in the source code in com.ibm.wala.cast.java.test.data. com.ibm.wala.cast.java.ecj does this by changing its working directory for running tests:

https://github.com/wala/WALA/blob/748b55661f47c099288374d0d4de734916bb714f/com.ibm.wala.cast.java.ecj/build.gradle#L52

com.ibm.wala.ide.jdt.test relies on the sources getting zipped up into a test_project.zip file (by running an Ant script) which then gets imported as an Eclipse project. It's this last part that is a bit tricky to handle via Maven artifacts. But, if we create a Maven artifact for com.ibm.wala.cast.java.test.data, we could probably write a (Gradle?) script in the new repo that depends on the sources jar for that artifact and re-creates the test_project.zip file appropriately. It'll take some care to make sure the right manifest file is there, etc. But it should be doable. The advantage of this (as opposed to my previous idea) is it would let us delete more old build scripts in the WALA repo (another pom.xml file and a build.xml file).

Anyway, thoughts / feedback welcome.

msridhar avatar Sep 11 '21 17:09 msridhar

A note: there is some code in com.ibm.wala.ide.tests that is runnable under Gradle and that I would like to keep working with the latest WALA master; see #965. We may need to do some restructuring to separate out code that relies on running in an Eclipse plugin (and can only be tested under Maven) from code that just relies on SWT and is runnable under Gradle.

msridhar avatar Sep 12 '21 22:09 msridhar