search

wala / WALA

T.J. Watson Libraries for Analysis, with frontends for Java, Android, and JavaScript, and may common static program analyses
http://github.com/wala/WALA
Eclipse Public License 2.0
763 stars 222 forks source link

Merge test subprojects into corresponding non-test subprojects #636

Closed liblit closed 4 years ago

liblit commented 4 years ago

WALA has many subprojects whose names end in .test or some variation thereof. Each of these could potentially be merged into the corresponding non-test subproject. This would result in a tidier source tree that conforms more closely to Java and Gradle conventions.

I raised this idea in a discussion of a large path-standardization pull request, but also raised doubts as to its feasibility. @msridhar expressed support and suggested a preliminary exploration. I propose that we continue the discussion here so that we have an easy-to-find record of what we decide to do and why.

liblit commented 4 years ago

Background

I have created a quick-and-dirty version of this restructuring: good enough to test buildability but without trying to get tests to pass. Creating this rough draft reminded me of the Eclipse limitations I ran into the first time I looked into this possibility.

Gradle has a much richer model of dependencies than Eclipse, which forces Eclipse’s Buildship plugin to make certain approximations. The effect of these approximations is to combine Gradle’s main and test dependencies for any given subproject, and likewise to combine compile- and run-time-dependencies. These conflations can create the false appearance of a dependency cycle, such as when foo’s test code depends on bar’s main code, which in turn depends on foo’s main code. As seen by Eclipse, this looks like foo depending on bar depending on foo: a disallowed cycle.

Easy to Merge

If we wish to continue to support WALA development in Eclipse, then we won’t be able to merge every such subproject. But we could merge many of them. The following subprojects could probably be merged:

After merging the combined project would have most source files gathered into some subset of src/{main,test}/{java,resources}, per default Gradle Java conventions. Tidy!

Not Easy to Merge

However, other subprojects cannot be merged with moderate effort. For notation convenience, use foo[bar] to represent the bar sourceSet of subproject foo. Then the following merge attempts run into problems due to false cycles arising from Eclipse Buildship conflation of dependencies:

Additionally, the following merge attempts had other troubles unrelated to Eclipse Buildship conflation of dependencies:

Next Steps

So overall we have seven test projects that can probably be merged and five that cannot. Worth doing? I think yes. @msridhar, I welcome your thoughts on all of this, especially on whether you think any of the five I cannot easily merge could be made easy-to-merge after all with moderate effort.

msridhar commented 4 years ago

Thanks for trying this out @liblit! A few thoughts / concerns.

Gradle has a much richer model of dependencies than Eclipse, which forces Eclipse’s Buildship plugin to make certain approximations. The effect of these approximations is to combine Gradle’s main and test dependencies for any given subproject, and likewise to combine compile- and run-time-dependencies.

Even if we get everything working, this seems like it could lead to a bad experience for Eclipse users. In particular, there could be differences between using Eclipse's built-in build system and building with Gradle due to differing dependencies. E.g., if you are hacking on non-test code, in smart completion you may see suggestions from test dependencies, and everything will look fine inside Eclipse's build. Then, when you try to build with Gradle, the code won't compile since that dependence is not present.

I wonder if this issue is significant enough that maybe we should avoid this change. Thoughts?

Let's ignore the above for now and move on.

Not Easy to Merge

However, other subprojects cannot be merged with moderate effort. For notation convenience, use foo[bar] to represent the bar sourceSet of subproject foo. Then the following merge attempts run into problems due to false cycles arising from Eclipse Buildship conflation of dependencies:

  • com.ibm.wala.core.tests ⇏ com.ibm.wala.core

    • com.ibm.wala.testutil[main] depends on com.ibm.wala.core[main] at compile time
    • com.ibm.wala.core[test] depends on com.ibm.wala.testutil[main] at compile time

This one could be solved by pulling testutil into wala.core as a test fixture I think; it's a good use of it.

  • com.ibm.wala.ide.tests ⇏ com.ibm.wala.ide

    • com.ibm.wala.ide[test] depends on com.ibm.wala.ide.jdt[main] at run time
    • com.ibm.wala.ide.jdt[main] depends on com.ibm.wala.ide[main] at compile time

The com.ibm.wala.ide[test] to com.ibm.wala.ide.jdt[main] seems like a mistake or bad. Can we move some code around to get rid of it?

  • com.ibm.wala.ide.jdt.test ⇏ com.ibm.wala.ide.jdt

    • com.ibm.wala.ide.jdt[test] depends on com.ibm.wala.ide.jdt[main] at compile time
    • com.ibm.wala.ide.jdt[main] depends on com.ibm.wala.ide[test] at compile time
    • com.ibm.wala.ide[test] depends on com.ibm.wala.ide.jdt[main] at run time

Same as the previous problem I think.

Additionally, the following merge attempts had other troubles unrelated to Eclipse Buildship conflation of dependencies:

  • com.ibm.wala.cast.java.test ⇏ com.ibm.wala.cast.java

    • unclear how to reconcile non-test sources in com.ibm.wala.cast.java.test/src/main with existing sources in com.ibm.wala.cast.java/src/main

This one I don't fully understand. Maybe we can put it aside for now.

  • com.ibm.wala.cast.java.test.data

    • unclear whether this should go into com.ibm.wala.cast.java or com.ibm.wala.cast.java.test
    • still includes Maven build files; I am loath to touch anything that still uses Maven

Yeah I'd stay away from this one for now due to Maven. I think we could probably (?) port the Maven stuff to Gradle, at which point we could revisit.

Next Steps

So overall we have seven test projects that can probably be merged and five that cannot. Worth doing? I think yes. @msridhar, I welcome your thoughts on all of this, especially on whether you think any of the five I cannot easily merge could be made easy-to-merge after all with moderate effort.

I think the first question I asked regarding development experience for Eclipse users is probably the most important. @liblit let me know your thoughts on that.

msridhar commented 4 years ago

Does the fix for https://github.com/eclipse/buildship/issues/689 help us here?

liblit commented 4 years ago

@msridhar wrote:

I think the first question I asked regarding development experience for Eclipse users is probably the most important. @liblit let me know your thoughts on that.

Nice thinking, @msridhar. Fortunately, a quick experiment shows that Eclipse is better at main/test segregation than I thought:

  1. I added a field to a main class whose type was a class from the same subproject’s test code. Eclipse correctly marked that type as unresolvable, with the expected wavy red underline.

  2. I added a field to a test class whose type was a class from the same subproject’s main code. Eclipse correctly resolved that type.

So Eclipse lets test code see main code, but main code cannot see test code: that is exactly as it should be. And yes, the fix for eclipse/buildship#689 is certainly part of what is letting this work.

msridhar commented 4 years ago

How about testCompile dependencies like jUnit? Will Eclipse allow main code to see classes in those?

liblit commented 4 years ago

Eclipse correctly flags org.junit as unresolvable when I add import org.junit.Test; to a main source file.

Thank you for asking good questions, @msridhar!

msridhar commented 4 years ago

That’s great! So then it seems that Eclipse now does understand the difference between main and test dependencies. At least I can’t think of what other problem is there.

So I think we can start merging these projects. But I’m not sure how to order the work. @liblit do you think it’s worth trying to fix the remaining blockers (like with testutil) so we can try to do all the renaming / merging of projects at once?

liblit commented 4 years ago

@msridhar wrote:

So then it seems that Eclipse now does understand the difference between main and test dependencies.

Indeed, this is much better than it used to be. There’s still the problem of false cycles, though. My current understanding is that Eclipse requires an acrylic dependency graph among subprojects, whereas IntelliJ IDEA and Gradle only require acyclic dependencies among subproject sourceSets.

@liblit do you think it’s worth trying to fix the remaining blockers (like with testutil) so we can try to do all the renaming / merging of projects at once?

Yes, that would be kindest. If these mass restructurings are disruptive for any other WALA users out there, I’d rather impose this pain on them just once rather than twice or more.

My rough-draft merging experiment used the pending changes from #635 as its starting point. I think that’s the right way to approach any attempt to resolve the remaining blockers: begin with #635 and tweak as needed. Once we have the remaining blockers resolved (or have given up on some as unresolvable), then I will redo the merge in a more orderly, non-experimental manner.

@msridhar, care to lend a hand on some of those blockers? It seems you already have strategies in mind akin to other work you recently did. I’d be happy to bring my #635 branch over into this WALA repository if that would be more convenient than where it currently lives in my personal WALA staging repository.

msridhar commented 4 years ago

@liblit I am very happy to help. Unfortunately with the start of the quarter coming I won't have much time to hack on this for a couple of weeks at least. The key outstanding tasks I think are:

Maybe we could make tasks on this and track it somehow? Like I said I won't be able to look for a while but this way we can track our progress.

And I'm happy to work on your fork if it's easier; either way is fine.

liblit commented 4 years ago

Maybe we could make tasks on this and track it somehow? Like I said I won't be able to look for a while but this way we can track our progress.

Yes, good idea. I have added #637, #638, and #639 to track each of these. We can use those to collect and preserve any further discussion on each, and use their assignees lists to avoid accidental redundant work. I have also created a milestone to gather those three new issues, this issue #636 (which depends on the other three), and pending pull request #635 (which we are postponing until #636 is also ready).

I have pushed two branches to the official WALA repository to contain our work:

This is all done with the understanding that I will be doing most of the code-slinging, consulting @msridhar as needed for opinions, advice, and pull request reviews.

liblit commented 4 years ago

@msridhar, I just noticed that the following subprojects still include Maven pom.xml and build.properties files:

Do you recommend leaving these alone for now, as you did for another subproject that still has Maven build configuration files?

msridhar commented 4 years ago

Yes, those should be left alone. Those are the files needed to run Eclipse Plug-in tests with Tycho in the Maven build. Until we figure out a non-Maven Tycho alternative they will need to stay in place.

msridhar commented 4 years ago

In fact, you may not want to do the merge-in of test projects under com.ibm.wala.ide.* for now; it may be tricky to keep the Maven build working if you do that.

liblit commented 4 years ago

Thanks for the guidance. I believe that those three are exactly the "test projects under com.ibm.wala.ide.*. So yeah, those stay as is for now.