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

Add functionality to create jar in zinc wrapper #6094

Merged
merged 11 commits into from Jul 24, 2018

Conversation

Projects
None yet
5 participants
@ity
Copy link
Member

ity commented Jul 11, 2018

Problem

Invocations of zinc do not support jars. See #6080 for details

Solution

Add functionality to the zinc wrapper to jar up the contents of _classesDirectory when an option is specified.

@ity ity requested review from baroquebobcat , stuhood and illicitonion Jul 11, 2018

@illicitonion
Copy link
Contributor

illicitonion left a comment

Looks great :) Didn't manage to plug this into the python code, but will hopefully try this out soon :)


val jarPath = Paths.get(classesDirectory.toString, settings.outputJar.toString)
val target = new JarOutputStream(Files.newOutputStream(jarPath))
val entryTime = System.currentTimeMillis()

This comment has been minimized.

@illicitonion

illicitonion Jul 11, 2018

Contributor

We should probably either have a flag to set the time to use (so that pants can set a fixed value, maybe falling back to the current time if no value is specified), or always set this to a fixed value, for determinism.

This comment has been minimized.

@ity

ity Jul 11, 2018

Member

absolutely!

FileVisitResult.CONTINUE
}

Files.walkFileTree(classesDirectory.toPath, FileVisitor)

This comment has been minimized.

@illicitonion

illicitonion Jul 11, 2018

Contributor

IIRC walkFileTree makes no guarantees about which order it visits files; we may want to manually list directory contents, sort them, and visit in sorted order, for determinism

This comment has been minimized.

@stuhood
@@ -57,12 +58,13 @@ case class Settings(

lazy val sources: Seq[File] = _sources map normalise

if (_classesDirectory.isEmpty && outputJar.isEmpty) {
throw new RuntimeException(
s"Either ${Settings.DestinationOpt} or ${Settings.JarDestinationOpt} option is required.")

This comment has been minimized.

@illicitonion

illicitonion Jul 11, 2018

Contributor

s/Either/At least one of/?

This comment has been minimized.

@stuhood

stuhood Jul 11, 2018

Member

I think either is accurate here.

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jul 11, 2018

Didn't manage to plug this into the python code, but will hopefully try this out soon

Unfortunately, doing that requires doing a local publish: there are instructions in the "dry-run" section here: https://www.pantsbuild.org/release_jvm.html#dry-run

1 similar comment
@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jul 11, 2018

Didn't manage to plug this into the python code, but will hopefully try this out soon

Unfortunately, doing that requires doing a local publish: there are instructions in the "dry-run" section here: https://www.pantsbuild.org/release_jvm.html#dry-run

/**
* Jar the contents of output classes (settings.classesDirectory) and copy to settings.outputJar
*/
def createClassesJar(settings: Settings, log: Logger) = {

This comment has been minimized.

@stuhood

stuhood Jul 11, 2018

Member

It might be a good idea to put this in a separate object somewhere (could preserve our silly naming convention here and call it OutputUtil or something, heh).

jarPath
}

val jarPath = Paths.get(classesDirectory.toString, settings.outputJar.toString)

This comment has been minimized.

@stuhood

stuhood Jul 11, 2018

Member

...I really can't believe that this is the recommended way to join paths in Java. But it is!

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 14, 2018

Contributor

I've found the ammonite ops library very neat for hygienic path operations (it has implicits that let you do e.g. Path(classesDirectory) / "intermediate-folder-i-just-made-up-for-this-example" / RelPath(settings.outputJar)). Its process creation mechanism isn't as useful as scala.sys.process for general use though, imho (although quite concise for synchronous process execution for testing). Not sure if I'm missing any reason that can't be used here.

*/
def createClassesJar(settings: Settings, log: Logger) = {
val classesDirectory = settings.classesDirectory
object FileVisitor extends SimpleFileVisitor[Path]() {

This comment has been minimized.

@stuhood

stuhood Jul 11, 2018

Member

Since this has mutable state inside, I'd recommend doing something like:

val jarCaptureVisitier = new SimpleFileVisitor[Path]() {
  ...
}
FileVisitResult.CONTINUE
}

Files.walkFileTree(classesDirectory.toPath, FileVisitor)

This comment has been minimized.

@stuhood
@@ -57,12 +58,13 @@ case class Settings(

lazy val sources: Seq[File] = _sources map normalise

if (_classesDirectory.isEmpty && outputJar.isEmpty) {
throw new RuntimeException(
s"Either ${Settings.DestinationOpt} or ${Settings.JarDestinationOpt} option is required.")

This comment has been minimized.

@stuhood

stuhood Jul 11, 2018

Member

I think either is accurate here.

@ity

This comment has been minimized.

Copy link
Member

ity commented Jul 13, 2018

Didn't manage to plug this into the python code, but will hopefully try this out soon

Unfortunately, doing that requires doing a local publish: there are instructions in the "dry-run" section here: https://www.pantsbuild.org/release_jvm.html#dry-run

Just saw this and realized why my test locally wasnt working. Since we need to publish, is there any way to write an integration test for this?

@illicitonion
Copy link
Contributor

illicitonion left a comment

Looks great!

I definitely defer to @stuhood for how to write tests here :)

@@ -121,6 +121,11 @@ object Main {
}

log.info("Compile success " + Util.timing(startTime))

// TODO(ity): if compile successful, jar the contents of classesDirectory and copy to outputJar

This comment has been minimized.

@illicitonion

illicitonion Jul 13, 2018

Contributor

Drop TODO?

@ity ity changed the title WIP - Add functionality to create jar in zinc wrapper Add functionality to create jar in zinc wrapper Jul 13, 2018

@stuhood
Copy link
Member

stuhood left a comment

Thanks Ity. Please add a quick unit test and then this should be good.

FileVisitResult.CONTINUE
}
sorted.map(createJar(_))
}

This comment has been minimized.

@stuhood

stuhood Jul 13, 2018

Member

This does not explicitly call close on the JarOutputStream, but should.

// deterministic jar creation
Files.walkFileTree(classesDirectory.toPath, fileSortVisitor)

val jarPath = Paths.get(classesDirectory.toString, settings.outputJar.toString)

This comment has been minimized.

@stuhood

stuhood Jul 13, 2018

Member

It looks like this will make this a relative path under the classesDirectory?

Both of these should probably be (assumed to be) absolute, so that they don't need to be located within one another.

This comment has been minimized.

@stuhood

stuhood Jul 17, 2018

Member

^ as mentioned below, I think that this is still a thing... the test is currently assuming this behaviour.

/**
* Jar the contents of output classes (settings.classesDirectory) and copy to settings.outputJar
*/
def createClassesJar(settings: Settings, log: Logger) = {

This comment has been minimized.

@stuhood

stuhood Jul 13, 2018

Member

I'd recommend creating one test that sanity checks this method: to make that easier, I'd recommend replacing Settings with explicit arguments for the classesDirectory and outputJar (and then maybe make the timestamp optional).

An example unit test for this code is over here: https://github.com/pantsbuild/pants/tree/master/tests/scala/org/pantsbuild/zinc/analysis

@@ -47,7 +48,8 @@ case class Settings(
compileOrder: CompileOrder = CompileOrder.Mixed,
sbt: SbtJars = SbtJars(),
_incOptions: IncOptions = IncOptions(),
analysis: AnalysisOptions = AnalysisOptions()
analysis: AnalysisOptions = AnalysisOptions(),
creationTime: Long = System.currentTimeMillis()

This comment has been minimized.

@stuhood

stuhood Jul 13, 2018

Member

This isn't a good default for this arg: would recommend 0 (the unix epoch) or something...

This comment has been minimized.

@ity

ity Jul 17, 2018

Member

I think if we want the jars to be deterministic (each JAR entry to have the same creation time), the default needs to be consistent for each JAR created but not the same as every other JAR created. lmk if I misunderstand

This comment has been minimized.

@stuhood

stuhood Jul 17, 2018

Member

but not the same as every other JAR created

Ideally "when" you create the jar does not matter at all. For the same inputs, creating the same jar twice (one+ second apart) should result in the exact same jar. Hence 0 here.

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jul 13, 2018

Since we need to publish, is there any way to write an integration test for this?

Only post merge, unfortunately.

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Jul 14, 2018

Only post merge, unfortunately.

Is this a "can't be done practically for reasons" or "someone needs to make this functionality"? It seems like publishing a jar is something that would be useful to have the ability to test (I'm thinking of a mixin or something which could then be extended for e.g. internal tasks which need to test publishing a jar for some reason). Is there context I'm missing?

@baroquebobcat
Copy link
Contributor

baroquebobcat left a comment

I've got one question below.

I'd also like to see a test that asserts things about the resulting jar contents. I'd be ok with deferring adding automated testing to the python side, but I have a strong preference for having a scala test if possible.


val fileSortVisitor = new SimpleFileVisitor[Path]() {
override def preVisitDirectory(path: Path, attrs: BasicFileAttributes): FileVisitResult = {
sorted.add(path)

This comment has been minimized.

@baroquebobcat

baroquebobcat Jul 17, 2018

Contributor

Are these paths relative to the classDirectory? If they're absolute, they might create the wrong entries in the jar file because they're used as the path for the JarEntry

@stuhood
Copy link
Member

stuhood left a comment

Thanks @ity !

I think that there is one remaining issue around relative paths. Should update the test to use two separate temp dirs before merging.

* @return
*/
def existsClass(jarPath: Path, clazz: String): Boolean = {
val jis = new JarInputStream(Files.newInputStream(jarPath))

This comment has been minimized.

@stuhood

stuhood Jul 17, 2018

Member

This stream needs to be closed (probably via try { .. } finally { .. }).

}
"JarCreationWithClasses" should {
"succeed when input classes are provided" in {
IO.withTemporaryDirectory { tempDir =>

This comment has been minimized.

@stuhood

stuhood Jul 17, 2018

Member

I think that the code and this test assume that the jar is always created under the classes directory... but that won't be the case.

Can you change this test to use two temporary directories: one containing the input classes, and another containing the output jar?

This comment has been minimized.

@ity

ity Jul 17, 2018

Member

I got rid of this assumption in the previous revision - added 2 separate dirs, PTAL when you can

val jarOutputPath = Paths.get(tempDirPath.toString, "spec-valid-output.jar")

OutputUtils.createJar(filePaths, jarOutputPath, System.currentTimeMillis())
OutputUtils.existsClass(jarOutputPath, tempFile.toString) must be(true)

This comment has been minimized.

@baroquebobcat

baroquebobcat Jul 17, 2018

Contributor

Thanks for writing the test. Things are a bit clearer to me now.

I think createJar should take a root for the input file paths, that way the entry paths can be relative to that root.

for example, if you had an input dir like temp-dir that looked like this.

temp-dir/org/example/Clazz.class
temp-dir/org/example/inner/Clazz2.class

I think you'd want the resulting jar to contain

org/example/Clazz.class
org/example/inner/Clazz2.class

I'm not sure it does this right now.

Does that make sense? I might be missing something.

@ity ity force-pushed the ity:ity/6080 branch from a50fa15 to ff5188f Jul 17, 2018

val jarOutputPath = Paths.get(tempOutputDir.toString, "spec-valid-output.jar")

OutputUtils.createJar(filePaths, jarOutputPath, System.currentTimeMillis())
OutputUtils.existsClass(jarOutputPath, tempFile.toString) must be(true)

This comment has been minimized.

@stuhood

stuhood Jul 17, 2018

Member

So, tempFile.toString is going to be an absolute filename, I think? In this case we're expecting literally Clazz.class.

@ity ity force-pushed the ity:ity/6080 branch from ff5188f to 5cedd46 Jul 18, 2018

@baroquebobcat
Copy link
Contributor

baroquebobcat left a comment

Thanks for updating the test and clearing up the relative path thing. It looks good to me now, apart from a copyright year that might need updating and possibly a method rename.

@@ -0,0 +1,12 @@
# Copyright 2017 Pants project contributors (see CONTRIBUTORS.md).

This comment has been minimized.

@baroquebobcat

baroquebobcat Jul 18, 2018

Contributor

nit: 2018


OutputUtils.createJar(tempInputDir.toString, filePaths, jarOutputPath, System.currentTimeMillis())
OutputUtils.existsClass(jarOutputPath, tempFile.toString) must be(false)
OutputUtils.existsClass(jarOutputPath, tempFile.getName) must be(true)

This comment has been minimized.

@baroquebobcat

baroquebobcat Jul 18, 2018

Contributor

Thanks 👍

This comment has been minimized.

@stuhood

stuhood Jul 18, 2018

Member

getName is only the final component of a filename, so this test would fail if the classfile were more deeply nested... it looks like the underlying code properly relativizes things though.

* Determines if a Class exists in a JAR provided.
*
* @param jarPath Absolute Path to the JAR being inspected
* @param clazz Name of the Class, the existence of which is to be inspected

This comment has been minimized.

@baroquebobcat

baroquebobcat Jul 18, 2018

Contributor

I think this function checks the existence of files in the jar and not the existence of classes by name. It might make sense to rename it and the clazz parameter to make that clearer.

jarEntry.setTime(jarEntryTime)

target.putNextEntry(jarEntry)
Files.copy(source, target)

This comment has been minimized.

@stuhood

stuhood Jul 18, 2018

Member

Hm... it looks like you're properly collecting directories above (ie, collecting directories in pre-visit), but I feel like this method will fail if the source: Path is a directory? It's important to actually store directory entries in the zip file, so I think that this case needs to add the jarEntry for a directory as a directory, and then skip appending the content.

You might consider having the FileVisitor store a tuple of (path: Path, isFile: Boolean) (which can still be sorted) to avoid needing to check whether the path is a directory here.

Between this and my comment in the test, I think the test needs an update to create a classfile under a directory.

This comment has been minimized.

@ity

ity Jul 18, 2018

Member

thanks for catching this - ptal when you can :)

@ity ity force-pushed the ity:ity/6080 branch from 5cedd46 to f7a8e6f Jul 18, 2018

@ity

This comment has been minimized.

Copy link
Member

ity commented Jul 18, 2018

Thanks for updating the test and clearing up the relative path thing. It looks good to me now, apart from a copyright year that might need updating and possibly a method rename.

👍

@stuhood
Copy link
Member

stuhood left a comment

I led you astray on one thing, sorry! Once that's fixed, please try out integrating with zinc, and then feel free to merge!


val fileSortVisitor = new SimpleFileVisitor[Path]() {
override def preVisitDirectory(path: Path, attrs: BasicFileAttributes): FileVisitResult = {
sorted.add(path, false)

This comment has been minimized.

@stuhood

stuhood Jul 18, 2018

Member

Eek. Sorry. Just looked at the javadocs for this: https://docs.oracle.com/javase/7/docs/api/java/util/zip/ZipEntry.html#isDirectory()

So the boolean isn't really necessary as long as you append a / here, and then later check whether it ends with a slash. Sorry!

This comment has been minimized.

@stuhood

stuhood Jul 18, 2018

Member

(...and the slash is required in order to not confuse consumers of the jar.)

@ity

This comment has been minimized.

Copy link
Member

ity commented Jul 19, 2018

I led you astray on one thing, sorry! Once that's fixed, please try out integrating with zinc, and then feel free to merge!

I should have read that myself really, thanks - been trying to test this locally but hitting some local caching issue with the zinc changes. will push once I have local integration tested.

@ity ity force-pushed the ity:ity/6080 branch from df624cb to 98afdd7 Jul 24, 2018

@ity ity merged commit b95a3a7 into pantsbuild:master Jul 24, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

CMLivingston pushed a commit to CMLivingston/pants that referenced this pull request Aug 27, 2018

Add functionality to create jars in zinc wrapper (pantsbuild#6094)
### Problem
Invocations of zinc do not support jars. 

### Solution
Add functionality to the zinc wrapper to jar up the contents of _classesDirectory when an option is specified.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment