Skip to content

Support -d with .jar paths #3382

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

Jump to bottom
Merged
merged 7 commits into from
Nov 16, 2017
Merged

Support -d with .jar paths #3382

merged 7 commits into from
Nov 16, 2017

Conversation

nicolasstucki
Copy link
Contributor

No description provided.

@nicolasstucki nicolasstucki mentioned this pull request Oct 25, 2017
Closed
@nicolasstucki nicolasstucki requested a review from smarter October 25, 2017 13:17
smarter
smarter reviewed Oct 26, 2017
View reviewed changes
compiler/src/dotty/tools/backend/jvm/GenBCode.scala Outdated
res
} else {
assert(output.hasExtension("jar"))
classOutput = new PlainDirectory(Path(Path(output.file).parent + "/tmp-jar-" + System.currentTimeMillis().toHexString).createDirectory())
Copy link
Member

Choose a reason for hiding this comment

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

scalac has code to write directly to a JAR instead of using a temporary directory which should be faster, maybe we can reuse that: https://github.com/scala/scala/blob/d41c97f13ea2348ec6f59c24ae098d83a96f30dc/src/compiler/scala/tools/nsc/backend/jvm/ClassfileWriter.scala#L23 Although apparently in Java >= 7 there's already an API dedicated to this: sbt/zinc#305 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ClassfileWriter came in the large refactoring made in scala/scala#6012. You said previously in #3113 that you would rather not port this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Java 7 API does not allow direct access to the files in the jar.

Once you have an instance of a zip file system, you can invoke the methods of the java.nio.file.FileSystem and java.nio.file.Path classes to perform operations such as copying, moving, and renaming files, as well as modifying file attributes. quote link

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't you do something like:

def outputDir(implicit ctx: Context): AbstractFile = {
  if (isJar) {
    val jar = Paths.get(pathTojar)
    val fs = FileSystems.newFileSystem(jar)
    new PlainNioFile(fs.getPath("/"))
  }
  ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will try

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It worked with the java.nio.file. FileSystem jar.

@nicolasstucki
Copy link
Contributor Author

Needs rebase on #3395 one merged

@allanrenucci allanrenucci self-assigned this Oct 30, 2017
@nicolasstucki nicolasstucki force-pushed the output-to-jar branch 6 times, most recently from dfb822a to 9018fb5 Compare November 7, 2017 10:51
@nicolasstucki
Copy link
Contributor Author

Rebased

nicolasstucki
nicolasstucki commented Nov 15, 2017
View reviewed changes
compiler/src/dotty/tools/io/Path.scala Outdated
// changes the existing extension out for a new one, or adds it
// if the current path has none.
def changeExtension(ext: String): Path =
if (extension == "") addExtension(ext)
else Path(path.stripSuffix(extension) + ext)
else new Path(jpath.resolveSibling(stripExtension + "." + extension))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

new Path(jpath.resolveSibling(stripExtension + "." + ext))

compiler/src/dotty/tools/io/JarFS.scala Outdated
Files.createDirectories(path.getParent)
Files.newOutputStream(path)
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

:)

compiler/src/dotty/tools/backend/jvm/GenBCode.scala Outdated
path.delete()
val path = Directory(ctx.settings.outputDir.value)
if (path.extension == "jar") {
if (jarFS == null)
jarFS = JarFS.create(path)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should make sure that the jar is empty or all files are rewritten.

nicolasstucki
nicolasstucki commented Nov 15, 2017
View reviewed changes
compiler/src/dotty/tools/io/JarFS.scala Outdated
object JarFS {
def create(path: Path): JarFS = {
assert(path.extension == "jar")
val env = Map("create" -> "true").asJava
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@allanrenucci here is the documentation:
https://docs.oracle.com/javase/7/docs/technotes/guides/io/fsp/zipfilesystemprovider.html
https://docs.oracle.com/javase/7/docs/technotes/guides/io/fsp/zipfilesystemproviderprops.html

Copy link
Member

Choose a reason for hiding this comment

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

I would add these links in comments if they're useful to understand the code

@allanrenucci
Copy link
Contributor

test performance please

@dottybot
Copy link
Member

performance test scheduled: 1 job(s) in queue, 0 running.

smarter
smarter reviewed Nov 15, 2017
View reviewed changes
compiler/src/dotty/tools/io/JarFS.scala Outdated

import scala.collection.JavaConverters._

class JarFS private (private[this] var jarFS: FileSystem) {
Copy link
Member

Choose a reason for hiding this comment

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

Please add some documentation :).

@dottybot
Copy link
Member

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/3382/ to see the changes.

Benchmarks is based on merging with master (7759e00)

@nicolasstucki
Copy link
Contributor Author

test performance please

@dottybot
Copy link
Member

performance test scheduled: 1 job(s) in queue, 0 running.

@dottybot
Copy link
Member

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/3382/ to see the changes.

Benchmarks is based on merging with master (7759e00)

@allanrenucci
Copy link
Contributor

We definitely got a speed up with this PR. It was not intentional though 😂

@smarter
Copy link
Member

smarter commented Nov 15, 2017

Where does the speed-up comes from?

@nicolasstucki
Copy link
Contributor Author

We removed implicit conversion.

@nicolasstucki
Copy link
Contributor Author

@allanrenucci parts LGTM

@allanrenucci allanrenucci merged commit 40011e2 into scala:master Nov 16, 2017
@allanrenucci allanrenucci deleted the output-to-jar branch November 16, 2017 13:21
@griggt griggt mentioned this pull request Mar 8, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smarter smarter smarter left review comments

@allanrenucci allanrenucci allanrenucci approved these changes

Assignees

@allanrenucci allanrenucci

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nicolasstucki @allanrenucci @dottybot @smarter