Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support alternate JavaCompilers (such as ECJ) in CompilationRule #71

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tbroyer
Copy link

@tbroyer tbroyer commented Jul 11, 2015

ECJ expects all JavaFileObjects to be Files, so the rule no longer
uses Compilation.compile() but instead directly uses the JavaCompiler,
and processes a class rather than compiling a dummy (in-memory) source.

@doanduyhai
Copy link

It does not work for me when I use the Rule:

    @Rule
    public CompilationRule compilationRule = new CompilationRule(() -> new EclipseCompiler());

By debugging, I realized that in the Compilation class, we are still fetching the compiler from ToolProvider.getSystemJavaCompiler() and not from the CompilationRule ...

@tbroyer
Copy link
Author

tbroyer commented Nov 16, 2015

I think you're confusing two things: the CompilationRule and the Truth assertions. The CompilationRule doesn't change the behavior of the assertions.

@doanduyhai
Copy link

@tbroyer You're right, what I need is more something like:

 Truth.ASSERT.about(JavaSourceSubjectFactory.javaSource())
                .that(loadClass(XXX.class))
                .withCompiler(new EclipseCompiler())
                .withCompilerOptions("-nowarn", "-1.8")
                .processedWith(this)
                .compilesWithoutError();

For that, we need to add a new method withCompiler() to the JavaSourcesSubject class

I'll do a pull request for that

@gk5885
Copy link
Contributor

gk5885 commented Nov 25, 2015

Hey, @tbroyer. Sorry it took me quite so long to get back to you. The biggest problem with this p/r is that we can't use a file-based file manager (at least for the default). For most small projects, whether your tests access the disk or not is inconsequential, but for our test infrastructure it makes a very big difference in the aggregate.

I hate making special provisions just for how broken ecj is, but this might be a reasonable place to do a reflective check and fall back to their file manager iff we know it's eclipse.

Seem reasonable?

@tbroyer
Copy link
Author

tbroyer commented Nov 26, 2015

Hmm, not sure I follow… The tests won't reach the disk (and wrapping the file manager in an InMemoryJavaFileManager works the same, it just happens to be useless because we never actually use it, as the CompilationRule uses a processor that doesn't output anything).
Actually, the real deal-breaker with ECJ is the fact that we tried to compile a dummy class that isn't file-based (JavaFileObjects.forSourceLines("Dummy", "final class Dummy {}"); which leads to an java.lang.IllegalArgumentException: URI has an authority component when ECJ tries to create a java.io.File). Switching the rule to processing an already-compiled class (CompilationRule.class.getName() here) fixes the issue; but that would mean either changing the Compilation.compile() API with that new argument, or introducing a Compilation.process() method. I went with the option to copy only the necessary bits from Compilation.compile(), removing anything that wasn't useful for the CompilationRule (namely the Result and InMemoryJavaFileManager, and passing some nulls instead of empty sets).

@googlebot
Copy link
Collaborator

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

tbroyer and others added 2 commits March 14, 2016 00:12
ECJ expects all JavaFileObjects to be Files, so the rule no longer
uses Compilation.compile() but instead directly uses the JavaCompiler,
and processes a class rather than compiling a dummy (in-memory) source.
@tbroyer tbroyer force-pushed the ecj-in-compilationrule branch from 35ae925 to c09dd40 Compare March 13, 2016 23:27
@tbroyer
Copy link
Author

tbroyer commented Mar 13, 2016

Rebased, and cherry-picked @doanduyhai's commit from #80 (effectively merging both PRs). PTAL.

@cpovirk
Copy link
Member

cpovirk commented Oct 2, 2019

I don't know the status of this, but for CLA purposes:

tbroyer appears to be the only author now: c09dd40 leads to https://github.com/google/compile-testing/commits?author=doanduyhai ("No commits found for 'doanduyhai'")

@cpovirk cpovirk added cla: yes and removed cla: no labels Oct 2, 2019
@cpovirk cpovirk added P4 type=addition A new feature labels Oct 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants