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.

Sign up for GitHub

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

Re-shade zinc to avoid classpath collisions with annotation processors. #5953

Merged
merged 1 commit into from
Jun 12, 2018
Merged

Re-shade zinc to avoid classpath collisions with annotation processors. #5953

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/python/pants/backend/jvm/subsystems/zinc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,13 +49,17 @@ def register_options(cls, register):
Shader.exclude_package('scala', recursive=True),
Shader.exclude_package('xsbt', recursive=True),
Shader.exclude_package('xsbti', recursive=True),
# Unfortunately, is loaded reflectively by the compiler.
Shader.exclude_package('org.apache.logging.log4j', recursive=True),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that mean that when compiling sources that depend on a different version of log4j, there could be collision or binary incompatibility?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Unavoidable, as far as I can tell. I confirmed that zinc fails to run without this exception.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine shipping this, but maybe worth sandboxing this internally first and see how much pain it might cause if any?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wisechengyi : This only affects annotation processors, of which we have relatively few. I've tested it with them.

]

cls.register_jvm_tool(register,
Zinc.ZINC_COMPILER_TOOL_NAME,
classpath=[
JarDependency('org.pantsbuild', 'zinc-compiler_2.11', '0.0.5'),
])
],
main=Zinc.ZINC_COMPILE_MAIN,
custom_rules=shader_rules)

cls.register_jvm_tool(register,
'compiler-bridge',
Expand Down Expand Up @@ -85,7 +89,7 @@ def register_options(cls, register):

@classmethod
def _zinc(cls, products):
return cls.tool_classpath_from_products(products, Zinc.ZINC_COMPILER_TOOL_NAME, cls.options_scope)
return cls.tool_jar_from_products(products, Zinc.ZINC_COMPILER_TOOL_NAME, cls.options_scope)

@classmethod
def _compiler_bridge(cls, products):
Expand Down
4 changes: 2 additions & 2 deletions src/python/pants/backend/jvm/tasks/jvm_compile/zinc/zinc_compile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,7 @@ def _zinc_cache_dir(self):
than compiling it.
"""
hasher = sha1()
for cp_entry in self._zinc.zinc + [self._zinc.compiler_interface, self._zinc.compiler_bridge]:
for cp_entry in [self._zinc.zinc, self._zinc.compiler_interface, self._zinc.compiler_bridge]:
hasher.update(os.path.relpath(cp_entry, self.get_options().pants_workdir))
key = hasher.hexdigest()[:12]
return os.path.join(self.get_options().pants_bootstrapdir, 'zinc', key)
Expand Down Expand Up @@ -353,7 +353,7 @@ def compile(self, ctx, args, classpath, upstream_analysis,
fp.write(arg)
fp.write(b'\n')

if self.runjava(classpath=self._zinc.zinc,
if self.runjava(classpath=[self._zinc.zinc],
main=Zinc.ZINC_COMPILE_MAIN,
jvm_options=jvm_options,
args=zinc_args,
Expand Down
7 changes: 7 additions & 0 deletions testprojects/3rdparty/com/google/guava/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# NB: This is an intentionally ancient guava to attempt to flush out classpath collisions
# with the compiler.
jar_library(name='ancient-guava',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the guava zinc uses is already shaded, why do we still need to worry about classpath collision for the testproject?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're attempting to cause an incompatible collision here, which might catch bugs (like the lack of shading) in the future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see that's what you mean by

flush out classpath collisions

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unclear to me why an ancient guava would flush out classpath problems. Is Zinc using an ancient version?

jars=[
jar('com.google.guava', 'guava', '10.0')
])

4 changes: 3 additions & 1 deletion testprojects/src/java/org/pantsbuild/testproject/annotation/processor/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@ annotation_processor(
sources=globs('*.java'),
processors=['org.pantsbuild.testproject.annotation.processor.ResourceMappingProcessor'],
dependencies=[
'3rdparty:guava',
# NB: We use the oldest guava we can stomach here, in order to flush out classpath
# collisions between the compiler and annotation processors.
'testprojects/3rdparty/com/google/guava:ancient-guava',
],
)

32 changes: 16 additions & 16 deletions ...ts/src/java/org/pantsbuild/testproject/annotation/processor/ResourceMappingProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
package org.pantsbuild.testproject.annotation.processor;

import com.google.common.collect.ImmutableSet;
import com.google.common.io.Closer;
import java.io.Closeable;
import java.io.IOException;
import java.io.PrintWriter;
Expand Down Expand Up @@ -120,24 +119,25 @@ public boolean process(Set<? extends TypeElement> annotations, RoundEnvironment
}

FileObject outputFile = createResourceOrDie("" /* no package */, REPORT_FILE_NAME);
Closer closer = Closer.create();
try {
Set<String> typeNames = new HashSet<String>();
PrintWriter writer = closer.register(new PrintWriter(outputFile.openWriter()));
for (TypeElement annotation : annotations) {
Set<? extends Element> annotatedElements = roundEnv.getElementsAnnotatedWith(annotation);
Set<TypeElement> typeElements = ElementFilter.typesIn(annotatedElements);
for (TypeElement typeElement : typeElements) {
String typeName = elementUtils.getBinaryName(typeElement).toString();
typeNames.add(typeName);
writer.println(typeName);
PrintWriter writer = new PrintWriter(outputFile.openWriter());
try {
Set<String> typeNames = new HashSet<String>();
for (TypeElement annotation : annotations) {
Set<? extends Element> annotatedElements = roundEnv.getElementsAnnotatedWith(annotation);
Set<TypeElement> typeElements = ElementFilter.typesIn(annotatedElements);
for (TypeElement typeElement : typeElements) {
String typeName = elementUtils.getBinaryName(typeElement).toString();
typeNames.add(typeName);
writer.println(typeName);
}
}
}

closer.close();

writeResourceMapping(typeNames, outputFile);
log(Diagnostic.Kind.NOTE, "Generated resource '%s'", outputFile.toUri());
writeResourceMapping(typeNames, outputFile);
log(Diagnostic.Kind.NOTE, "Generated resource '%s'", outputFile.toUri());
} finally {
writer.close();
}
} catch (IOException e) {
throw new RuntimeException(e);
}
Expand Down