Skip to content

Fix regression in ZipArchive#9535

Merged
dwijnand merged 1 commit intoscala:2.13.xfrom
jozic:fix-zip-archive
Mar 11, 2021
Merged

Fix regression in ZipArchive#9535
dwijnand merged 1 commit intoscala:2.13.xfrom
jozic:fix-zip-archive

Conversation

@jozic
Copy link
Contributor

@jozic jozic commented Mar 7, 2021

When I tried to upgrade from Scala 2.13.2 to Scala 2.13.3-5 I started to get strange compiler error.
After some debug I figured that some of our dependency jars in the root have files which names start with /.
So I added a simple unit test that on 2.13.x fails with

java.lang.StringIndexOutOfBoundsException: String index out of range: -1

	at java.lang.String.charAt(String.java:658)
	at scala.reflect.io.ZipArchive$.splitPath(ZipArchive.scala:61)
	at scala.reflect.io.ZipArchive$.scala$reflect$io$ZipArchive$$dirName(ZipArchive.scala:58)
	at scala.reflect.io.ZipArchive.ensureDir$1(ZipArchive.scala:131)
	at scala.reflect.io.ZipArchive.getDir(ZipArchive.scala:139)
	at scala.reflect.io.FileZipArchive.root$lzycompute(ZipArchive.scala:219)
	at scala.reflect.io.FileZipArchive.root(ZipArchive.scala:204)
	at scala.reflect.io.FileZipArchive.iterator(ZipArchive.scala:242)
	at scala.reflect.io.ZipArchiveTest.$anonfun$weirdFileAtRoot$4(ZipArchiveTest.scala:42)
	at scala.reflect.io.ZipArchiveTest.$anonfun$weirdFileAtRoot$4$adapted(ZipArchiveTest.scala:41)
	at scala.util.Using$.$anonfun$resources$2(Using.scala:298)
	at scala.util.Using$.resource(Using.scala:261)
	at scala.util.Using$.$anonfun$resources$1(Using.scala:297)
	at scala.util.Using$.resource(Using.scala:261)
	at scala.util.Using$.resources(Using.scala:296)
	at scala.reflect.io.ZipArchiveTest.weirdFileAtRoot(ZipArchiveTest.scala:41)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)

Which is exactly the error I get on 2.13.3+

Looks like this was introduced in 7068589#diff-3bf883c780984a2c8f1eda1e70bb6359c4696519142992a4ee7eedf7b6887000

I didn't find a bug like this reported, but maybe it exists.

@scala-jenkins scala-jenkins added this to the 2.13.6 milestone Mar 7, 2021
@jozic
Copy link
Contributor Author

jozic commented Mar 7, 2021

@retronym please check it out

dwijnand
dwijnand approved these changes Mar 8, 2021
View reviewed changes
Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

LGTM

@SethTisue
Copy link
Member

SethTisue commented Mar 9, 2021

t7020.scala is failing — @dwijnand would that go away with a rebase?

@dwijnand
Copy link
Member

dwijnand commented Mar 9, 2021

It might but not necessarily.

@som-snytt
Copy link
Contributor

som-snytt commented Mar 11, 2021

It also fails on 2.12, so here is a backport.

#9541

Pandemic code review is hard. I see I mentioned something about the forward merge colliding with my related fix. I kind of remember staring at this code, but missed the bug. Also 2 or 3 other comments came up. What about the JDK OPT to look up foo/ together with foo?

@som-snytt som-snytt mentioned this pull request Mar 11, 2021
Merged
@som-snytt
Copy link
Contributor

som-snytt commented Mar 11, 2021 •
edited
Loading

I was about to say I'd have to read more doc.

4.4.17 file name: (Variable)

   4.4.17.1 The name of the file, with optional relative path.
   The path stored MUST NOT contain a drive or
   device letter, or a leading slash.  All slashes
   MUST be forward slashes '/' as opposed to
   backwards slashes '\' for compatibility with Amiga
   and UNIX file systems etc.  If input came from standard
   input, there is no file name field.

At https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT via https://www.loc.gov/preservation/digital/formats/fdd/fdd000354.shtml

@jozic
Copy link
Contributor Author

jozic commented Mar 11, 2021

how come backport PR passes all the checks and this PR doesn't? should I rebase on lastest 2.13.x?

@som-snytt
Copy link
Contributor

som-snytt commented Mar 11, 2021

I think they were saying there is a so-called flaky test on 2.13. Kind of makes you wish you had PRd against 2.12.

@SethTisue
Copy link
Member

SethTisue commented Mar 11, 2021 •
edited
Loading

yeah rebasing this onto current 2.13.x should fix (by pulling in #9540)

@lrytz lrytz force-pushed the fix-zip-archive branch from d9c5839 to 411d548 Compare March 11, 2021 08:21
@dwijnand dwijnand enabled auto-merge March 11, 2021 08:25
@dwijnand dwijnand merged commit e0f069a into scala:2.13.x Mar 11, 2021
@jozic jozic deleted the fix-zip-archive branch March 11, 2021 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dwijnand dwijnand dwijnand approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2.13.6

Development

Successfully merging this pull request may close these issues.

5 participants

@jozic @SethTisue @dwijnand @som-snytt @scala-jenkins