WIP: fix for build-outdated-file feature

[Egor18]

This is possible fix for issue #1685.
It removes UnsupportedOperationException, but it's still a bit unclear to me how spoon could perform incremental model build. I want to speed up model build by building only outdated files.

[pvojtechovsky]

but it's still a bit unclear to me how spoon could perform incremental model build.

It looks like incremental model build is not supported and tested by current spoon code.

You can try to move these lines from JDTBasedSpoonCompiler.java

		if (buildOnlyOutdatedFiles && outputDirectory.exists()) {
			@SuppressWarnings("unchecked") Collection<File> outputFiles = FileUtils.listFiles(outputDirectory, new String[] { "java" }, true);
			keepOutdatedFiles(sourceFiles, outputFiles);
		}

little bit up in the function, before this line

		JDTBatchCompiler batchCompiler = createBatchCompiler(new FileCompilerConfig(sourceFiles));

there is some little chance that it will work then, but there can be many other problems with incremental building. That eclipse compiler integration stuff is quite old and dark code. Anyway: "Everything is possible here", so give it some time and I am sure the incremental building can be implemented.

@monperrus WDYT?

[monperrus]

Thank you very much for this contribution.

In Spoon, when we fix a bug, we always commit a test case proving that it works with this fix (see Guideline G0).

In this case, it's even more important in this case because, as @pvojtechovsky points out, this is a rather untested part of Spoon.

@Egor18 would you add a test case to this PR?

[pvojtechovsky]

I agree. The test case is the first step to have it fixed. It is expected that this test will fail at the beginning. But thanks to that failing test case we will exactly see the use case you need to have solved!

[Egor18]

I added test for this issue. Without my fix this test fails with:
[ERROR] IncrementalBuildTest.testIncrementalBuild:17 » UnsupportedOperationException

P.S. Sorry for the mess with commits. Probably you can squash them.

[pvojtechovsky]

Hi Egor18, many thanks for your time you spent on that!

Sorry for the mess with commits.

We welcome everybody interested in Spoon. It is really no problem if PR contains more commits or is not ideal since beginning. It is no problem to discuss solution and to improve it to the desired quality.

I read your test and I do not see anything related to incremental building. I though that your use case is something like:

1. build spoon model from many java files (in case of test at least two files)
2. modify some java files (in case of test at least one file is modified)
3. build spoon model again using incremental build (it should parse and update only part of spoon model related to modified files)
4. check that whole spoon model contains expected information: old not modified model + updated model of modified file(s)

Do I understand well your use case? Could you make such test? Or is it completely different in your case?

[Egor18]

Well, actually test works with two directories: in and out.
in - original input source files
out - spooned files (I simply added .java file in there to "simulate" the second build).

So test represents situation, when the model was already built once (and spooned files were generated), and we are building it again. This test is only considering UnsupportedOperationException.

I'll try to test the actual incremental build a bit later.

[pvojtechovsky]

This test is only considering UnsupportedOperationException.

OK, I see that this test reproduce the problem with UnsupportedOperationException.
I checked the spoon code, and like you, I see that incremental building is not implemented/not implement well. So I think , that it makes no sense to fix only UnsupportedOperationException without improving all spoon code related to incremental building. Therefore I would prefer to start with a test case which really tests incremental building.

[Egor18]

I added the actual incremental build test. As expected, it fails. The number of types in the incrementally built model is not equal to the actual number of types. So this model does not contain info from old files.

[surli]

Hi @Egor18

Thanks for fixing the bug with unmodifiable list.
I think your unit test should be modified as the contract you want is another feature in Spoon.
Currently when you want to build model of outdated file you only obtain the content of those files inside the model (so, only your file First.java in your case). I think this feature has been made mainly in order to write only changed files, so to be use with launcher.run() and not necessarily with launcher.buildModel().

I propose that you change your test here to make it passing (just change your assert), then we can merge your bug fix with lists.
And then submit a new PR containing the new contract, on which we can start working on for the new feature you want.

[pvojtechovsky]

Currently when you want to build model of outdated file you only obtain the content of those files inside the model (so, only your file First.java in your case)

I am not sure if current implementation is correct. What is the real benefit of build outdated files only?
A) to avoid parsing of java files and building of their model
B) to avoid pretty printing of java files
Note: that some processing algorithms might need to have loaded whole spoon model (even the not modified files) to be able to modify and then pretty print the modified files.

Note: At the beginning of this discussion I expected different use case: only one instance of Launcher. I expected that Spoon model made by first call of launcher.buildModel() is available at time when second call of launcher.buildModel() is called. Here during the second call, I would expect that we want to compile only modified files and that EXISTING spoon model is updated with new changes. Now I see that Egor18's use case is probably different and that there are many ways how to understand "build only outdated files".

May be we should first discuss, which use cases are really wanted and then we can implement them. It makes no sense to have implemented a feature/use case, which is wanted by nobody ... and just maintain it.

[surli]

May be we should first discuss, which use cases are really wanted and then we can implement them. It makes no sense to have implemented a feature/use case, which is wanted by nobody ... and just maintain it.

I completely agree with you. That was the meaning of my comment: there is a practical bug in the implementation for which we have a quick fix, and then let's continue the discussion about what we really want with the feature.

What is the real benefit of build outdated files only?

I see the benefit of not building the whole model, but as you pin point in some cases we need the whole model to be build in order to achieve transformation. Maybe a good trade-off would be to offer the capacity to serialize the Spoon model (e.g. in JSON) and then to reuse it to build incremental model: we would not have to parse again all java files, but we would keep the whole information of the model.

[pvojtechovsky]

there is a practical bug in the implementation for which we have a quick fix

I am just not sure if it is quick fix or change in implementation - incremental update never really worked. So may be this "quick fix" just brings a feature which nobody needs.

to serialize the Spoon model

I guess the Spoon model serialization is already supported. I have seen some test for that a year ago. It should serializes into java object stream.

[surli]

I am just not sure if it is quick fix or change in implementation

I'm just focused here on the problem of the unmodifiable list: it's clearly a bug. Even if the implementation is - for now - not exactly what users wants, it's surely better to not crash Spoon because of a wrong type of list.

[pvojtechovsky]

but change in unmodifiable list is not the only change which was done. There was moved some code so it changed behavior! But does it makes sense?

[msteinbeck]

Regarding incremental builds. I implemented a lib which reuses the compiled .class files of the last build by adding them to the class path of the next build. This ensures that the position information of the last build are not modified by the pretty print system of Spoon. I combined this approach with Spoon's noclasspath option making incremental builds very fast (I'm compiling about 4000 versions of a software system). Unfortunately, I can not share my code with you, but the concept.

1. Set the binary output directory of Spoon to a temporary directory.
2. Fetch your changes, for instance, by using diff
3. Remove all outdated types (and empty packages) from your AST.
4. Create a new Launcher resuing the Factory of your previous build.
5. Enable noclasspath build and add the generated .class files stored in the directory of 1)
6. Add your changed files and all files from the last build that have no associated .class file to the Launcher (this may happen if a file/class imports unknown dependencies)
7. Run the next build.

[Egor18]

In my opinion, it would be great to have the possibility to serialize full spoon model and store it on disk.
So next time we need to build the model, we could just modify this model, instead of full model rebuild.
Actually, I was expecting such behaviour. This feature could really speed up subsequent builds, because full model build takes quite a lot of time.

@msteinbeck's solution looks like ok, but as far as I understand, for unmodified files in subsequent builds it gets information from binaries (.class files). This means that some information about types would be incomplete. Case 2 from here.

[monperrus]

it would be great to have the possibility to serialize full spoon model and store it on disk

this is already supported, see https://github.com/INRIA/spoon/blob/master/src/test/java/spoon/test/serializable/SerializableTest.java

[msteinbeck]

@Egor18 I don't see why type information should be incomplete? In my approach you update the AST of the previous by yourself and since refrences are resolved using a factory instead of being hardcoded, type resolution should be fine.

[surli]

keepOutdatedFiles should not change the list of files but instead return the modified list. Its impact would be more clear.

[surli]

I'm not sure about moving this block up: we may need to give the whole bunch of files to JDT in order to avoid compilation error such as missing type, particularly in full-classpath mode.

[pvojtechovsky]

I agree with You! But the problem is that without that move the call of keepOutdatedFiles makes no sense at all because nobody is interested in the result of that call...
... the work with outdated files is broken in Spoon yet ... it is not only problem of unmodifiable collection ...

[Egor18]

So, should I write another test or something?

[pvojtechovsky]

So, should I write another test or something?

what next? I think we need a concept which defines how incremental build has to work. There are many ways how it might be done. @Egor18 I suggest that you define concept which fits exactly to your needs and then in case when the concept makes sense for other clients (not only for You), there is good chance that we implement it once. Or you can then provide a PR which implements this concept - we will gladly support you with that!

[monperrus]

Closing this one, the discussion has moved to #1816