Inconsistent handling of files in an output folder - full vs. incremental build #2921
Conversation
incremental build fixes eclipse-xtext#2920 Signed-off-by: Stephan Herrmann <stephan.herrmann@berlin.de>
|
I did poke around existing tests, to see if anything could be used as a template for creating a test challenging this change, but I failed to find any test that would even invoke |
|
i assume some of the xtend.ide tests test some of the usecases => you might check the history of code in xtext-eclipse there is also a builder test language in builder.tests. |
|
By copying from
In the last call, if the provider is a Going back to our original problematic case, I realize that we have a test case with a combination that's perhaps unsupported: we invoke the builder on a proper workspace, but we don't have UI, so we only get a Am I right that Xtext doesn't have a "mode" for a headless eclipse? Is that why only the "UI" variant asks If the answer is indeed "unsupported", then my change may be "correct but irrelevant". So would you still want it? @szarnekow wdyt? |
Signed-off-by: Stephan Herrmann <stephan.herrmann@berlin.de>
|
My assumption would be that even in headless eclipse with workspace how do you implement that |
|
@stephan-herrmann in such tests, do you use the UI injector provider? |
|
Our headless tests use a slim handcrafted All this is inspired by https://github.com/eclipse-jdt/eclipse.jdt.core/tree/master/org.eclipse.jdt.core.tests.builder which has no dependencies on anything ui :) DefaultResourceUIServiceProvider, OTOH, lives in org.eclipse.xtext.ui and hence will never work in a truly headless eclipse, where none of org.eclipse.ui* are available. Initially I had assumed the separation between o.e.xtext and o.e.xtext.ui can be compared to the separation between o.e.jdt.core and o.e.jdt.ui, but apparently xtext is not prepared to leverage jdt.core without any ui. As all this is only about minimizing tests (we don't ship headless applications), it is not a problem for production. |
|
if all the deps are optional xtext.ui still should live in an equinox env without workbench. |
but that's not the case 😄 - I see lots of non-optional ui dependencies in xtext.ui, so this plugin will simply not resolve in a truly headless environment.
fair enough. |
|
the funny thing is in the project i worked with andrey loskutov on it does. (of course with a bunch of extra bundles in the product) |
|
With all that I learned in this PR, I'd be fine with closing it without merge. Is anyone still planing to review? I mean it still seems to be the "right" thing to do, but the effective benefit might be low (a tiny performance improvement, more symmetric code). |
| } | ||
|
|
||
| @Override | ||
| public void build(IBuildContext context, IProgressMonitor monitor) throws CoreException { |
There was a problem hiding this comment.
@stephan-herrmann This test is not sufficient to validate that the fix actually fixes something. It's green when I run this on main. Do you plan to augment it somehow or would you like to merge this branch as is?
There was a problem hiding this comment.
Have you seen this comment?
In essence we'd need an environment with XtextBuilder but without DefaultResourceUIServiceProvider.
I can confirm that one of our tests, which uses DefaultResourceServiceProvider instead, does trigger the problem. Do you want me to construct such a test for xtext? It looked like that would need jumping through some hoops.
That's why I was uncertain whether we should actually pursue this further...
|
@stephan-herrmann maybe I didn't get the overall picture but if the problem is
To recreate your scenario, where you wouldn't want the could that recreate the problem and verify the fix correctly, @szarnekow ? |
This is correct, but I don't know if it's significantly easier to re-bind JdtHelper then IResourceServiceProvider. Frankly, I just didn't feel like creating a whole now language with its own guice module, assuming that would be overkill. |
|
I didn't mean create a new language: just use a custom ui injector provider, inheriting the one currently used. If you want I can have a try. |
that'd be cool, thanks. |
|
Apparently, I was too optimistic because I've never had problems in customizing test injectors for headless tests, but I never did that for UI tests. This does not work as expected: @RunWith(XtextRunner.class)
@InjectWith(GH2920Test.BuilderTestLanguageInjectorProviderCustom.class)
public class GH2920Test extends AbstractParticipatingBuilderTest {
private IJavaProject javaProject;
@Inject private IResourceDescriptionsProvider descriptionsProvider;
@Singleton
public static class JdtHelperCustom extends JdtHelper {
@Override
protected boolean computeJavaCoreAvailable() {
return false;
}
}
public static class BuilderTestLanguageInjectorProviderCustom extends BuilderTestLanguageInjectorProvider {
@Override
public Injector getInjector() {
Module mixin = Modules2.mixin(new BuilderTestLanguageRuntimeModule(),
new SharedStateModule() {
@Override
public Provider<IJdtHelper> provideJdtHelper() {
return new Provider<>() {
@Override
public IJdtHelper get() {
return new JdtHelperCustom();
}
};
}
},
new BuilderTestLanguageUiModule(TestsActivator.getInstance())
);
return Guice.createInjector(mixin);
}
}
...In fact, the custom injector is used only to inject things in the test case, but the workspace and other services, including the build participants, are injected with the executable extension factory, i.e., with the injector created in the test language activator, which cannot be customized. Maybe @szarnekow can shed some light on whether what I wanted to do is feasible without creating a new language. |
|
I think I found a solution to recreate the problem. I have to do a few clean up and then I'll tell you about that |
|
@stephan-herrmann I created a PR against your branch with the test reproducing the problem. Of course, in the PR the test succeeds, but if you try uncommenting your modifications to |
Test case that effectively demonstrates the problem the customization happens at the level of the executable extension factory
Signed-off-by: Stephan Herrmann <stephan.herrmann@berlin.de>
Thanks, @LorenzoBettini for going the extra mile! Now on top of your changes I proposed a variant that comes even closer to our original problem: in our case the IResourceServiceProvider was the point where behavior deviated, so I bound something that looks like an IResourceUIServiceProvider (because that's what is requested in the test setup) and behaves like a DefaultResourceServiceProvider. This, too, succeeds to demonstrate the problem when the actual fix is not in. If that's too much magic in the test, I'm fine with reverting this last commit. |
|
Thank you for the improved test. I like the approach! Let’s wait for the build to succeed and merge afterwards. |
fixes #2920