Skip to content
New issue

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

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

Already on GitHub? # to your account

In-memory shading #39

Merged
merged 12 commits into from
Oct 6, 2023
Merged

In-memory shading #39

merged 12 commits into from
Oct 6, 2023

Conversation

eed3si9n
Copy link
Owner

@eed3si9n eed3si9n commented Oct 6, 2023

Fixes #38

Problem

File name entry encoding issue.

Solution

  1. Generally use JarOutputStream or Jar* series of streams instead of Zip*. This is because PKWare spec says that the file names should be encoded using IBM Code Page 437, but JARs use UTF-8. JDK 7 adds a parameter to workaround this but generally JAR isn't ZIP in these details.
  2. Avoid unjarring, and perform in-memory shading. This would avoid the Unix path having issues with arbitrary string being a path.

Copy link
Collaborator

@johnynek johnynek left a comment

Choose a reason for hiding this comment

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

A few suggestions.

Thanks for doing this. Excited to reduce duplication or jarjar implementations and for the improved testing.

I wonder if you have any ideas for how we could set up CI to test with more than one time zone to verify that resulting shaded jars are time zone independent.

core/src/main/scala/com/eed3si9n/jarjarabrams/Zip.scala Outdated Show resolved Hide resolved
core/src/main/scala/com/eed3si9n/jarjarabrams/Zip.scala Outdated Show resolved Hide resolved
core/src/main/scala/com/eed3si9n/jarjarabrams/Zip.scala Outdated Show resolved Hide resolved
core/src/main/scala/com/eed3si9n/jarjarabrams/Zip.scala Outdated Show resolved Hide resolved
Copy link
Collaborator

@johnynek johnynek left a comment

Choose a reason for hiding this comment

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

A few more questions that maybe we can address later if you want.

core/src/main/scala/com/eed3si9n/jarjarabrams/Zip.scala Outdated Show resolved Hide resolved
entry.setCompressedSize(-1)
out.putNextEntry(entry)
out.write(struct.data)
} else if (struct.name.endsWith("/")) ()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this not be first? Not sure why it is here actually. The else of this if is also returning ()

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think we do want entries that ends with / to transform like shapeless/ becomes foo/shapeless. The original else if in Java code exempts directories from duplicate entry exception. If we ignore the duplicate entry exception, like I am doing here, then this clause is meaningless.

@eed3si9n
Copy link
Owner Author

eed3si9n commented Oct 6, 2023

@johnynek

I wonder if you have any ideas for how we could set up CI to test with more than one time zone to verify that resulting shaded jars are time zone independent.

sha256 might be a blunt hammer to check this, but it shows that I still have some stability issue:

[info]   assert(actualSha == expectedSha)
[info]          |         |  |
[info]          |         |  6e7372e52e3b2e981ffa42fc29d5cec1cc84431972d45fb51d605210e11c3ebd
[info]          |         false
[info]          038a68df151fc1549c487d79a5ae8ed5a1c2ff3f6ccbee2953f35bfc736b6e83

I don't think it's quite enforcing the minimal timestamp yet.

@@ -30,7 +30,7 @@ object Zip {
// --------- ---------- ----- ----
// 0 00-00-1980 04:08 META-INF/
// 988 00-00-1980 04:08 META-INF/MANIFEST.MF
private final val minimumTimestamp = 315705600L
private final val minimumTimestamp = 315705600000L
Copy link
Owner Author

Choose a reason for hiding this comment

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

I was off by 1000x (milisec).

@@ -65,6 +66,8 @@ object ShaderTest extends BasicTestSuite {
)
val entries = Zip.list(tempJar).map(_._1)
assert(entries.contains(expectedClass))
val lines = Process(s"unzip -l $tempJar").!!.linesIterator.toList.take(10)
lines.foreach(println)
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we remove now?

Copy link
Collaborator

@johnynek johnynek left a comment

Choose a reason for hiding this comment

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

When green let's merge!

@eed3si9n eed3si9n merged commit b0dbf0f into develop Oct 6, 2023
@eed3si9n eed3si9n deleted the wip/repro branch October 6, 2023 21:31
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Run CLI on Shapeless
2 participants