Skip to content

Less yammering, more hammering: fix two broken windows tests#8539

Merged
SethTisue merged 1 commit intoscala:2.13.xfrom
som-snytt:issue/zip-broken-windows
Nov 27, 2019
Merged

Less yammering, more hammering: fix two broken windows tests#8539
SethTisue merged 1 commit intoscala:2.13.xfrom
som-snytt:issue/zip-broken-windows

Conversation

@som-snytt
Copy link
Contributor

@som-snytt som-snytt commented Nov 12, 2019

Full disclosure: I haven't actually run this on windows yet.

ZipArchive is Autocloseable and used in Using expressions.

The ZipArchive should be closed (released) first, then the temp file should be deleted (released).

@scala-jenkins scala-jenkins added this to the 2.13.2 milestone Nov 12, 2019
@som-snytt som-snytt mentioned this pull request Nov 12, 2019
Merged
@som-snytt som-snytt force-pushed the issue/zip-broken-windows branch from dc139bb to 49fe654 Compare November 12, 2019 22:46
@som-snytt
Copy link
Contributor Author

som-snytt commented Nov 13, 2019

I broke my previous PR (personal record) and broke the type-tag-leak test in validate-main. On a roll.

@som-snytt som-snytt force-pushed the issue/zip-broken-windows branch from 49fe654 to c9c19e3 Compare November 13, 2019 03:41
@som-snytt
Copy link
Contributor Author

som-snytt commented Nov 13, 2019

The previous attempt to close everything still doesn't allow the file delete on windows, so the last change is not to attempt the delete on windows, so that tests can succeed for CI.

@som-snytt
Copy link
Contributor Author

som-snytt commented Nov 13, 2019

OK, since when is reflect.io a mima api?

@som-snytt som-snytt force-pushed the issue/zip-broken-windows branch from c9c19e3 to dc8c73b Compare November 13, 2019 05:32
@SethTisue
Copy link
Member

SethTisue commented Nov 13, 2019

queued: https://scala-ci.typesafe.com/job/scala-2.13.x-integrate-windows/1170/

@som-snytt
Copy link
Contributor Author

som-snytt commented Nov 13, 2019 •
edited
Loading

quoting fommil: "Why, oh why, Windows, must you put us through this?" the bug

I'll try the -D, but a glance at the code shows other opens, I can't tell if there are closes.

Edit: the -DcloseZip=true isn't used by ManifestResources. There is a comment on FileZipArchive.LeakyEntry for the trade-off.

@SethTisue
Copy link
Member

SethTisue commented Nov 13, 2019 •
edited
Loading

Travis green, Jenkins Windows green, Jenkins Linux has !! 1 - run/type-tag-leak.scala

@som-snytt
Copy link
Contributor Author

som-snytt commented Nov 13, 2019

I started looking at type-tag-leak, which is now a problem. I already learned about ClassValue!

There is no doubt a real solution for ManifestResources on Windows as for FileZipArchive. When I get a chance, now that I have the test set up on Windows, I'll write a test for each.

@lrytz lrytz mentioned this pull request Nov 18, 2019
Merged
lrytz
lrytz reviewed Nov 19, 2019
View reviewed changes
Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

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

OK, since when is reflect.io a mima api?

Only reflect.internal is excluded. Not sure reflect.io ends being used by macros.

@lrytz lrytz force-pushed the issue/zip-broken-windows branch from dc8c73b to e42a14d Compare November 19, 2019 10:34
@lrytz
Copy link
Member

lrytz commented Nov 19, 2019

(rebased for the type-tag-leak fix)

@SethTisue
Copy link
Member

SethTisue commented Nov 20, 2019

https://scala-ci.typesafe.com/job/scala-2.13.x-integrate-windows/1177/ has a different failure, !! 1 - run/t11802-pluginsdir

@som-snytt
Copy link
Contributor Author

som-snytt commented Nov 21, 2019

sbt completion doesn't work on windows in ordinary cmd. Oh, you mustn't allow it to autocomplete with forward slash, because that will break subsequent backslashing.

@som-snytt
Copy link
Contributor Author

som-snytt commented Nov 21, 2019

It must be a thing, and not an artifact of test environment.

It can't load the class:

##### Log file 'C:\Users\andre\Documents\scala\test\files\run\t11802-pluginsdir-run.log' from failed test #####

Error: unable to load class: t11802.SamplePloogin1
error: Missing required plugin: ploogin1_1

and also fails standalone:

C:\Users\andre\Documents\scala>dir test\files\run\t11802-pluginsdir-run.obj\myplugins
 Volume in drive C is OS
 Volume Serial Number is [REDACTED]

 Directory of C:\Users\andre\Documents\scala\test\files\run\t11802-pluginsdir-run.obj\myplugins

11/20/2019  04:24 PM    <DIR>          .
11/20/2019  04:24 PM    <DIR>          ..
11/20/2019  04:31 PM             7,821 p1.jar
               1 File(s)          7,821 bytes
               2 Dir(s)  124,528,775,168 bytes free

C:\Users\andre\Documents\scala>.\build\pack\bin\scalac -Vdebug -Xpluginsdir test\files\run\t11802-pluginsdir-run.obj\myplugins -Xplugin-require:ploogin1_1 -Vphases
error: Missing required plugin: ploogin1_1
    phase name  id  description
    ----------  --  -----------
        parser   1  parse source into ASTs, perform simple desugaring

@som-snytt
Copy link
Contributor Author

som-snytt commented Nov 21, 2019

The debug says everything is correct except the class isn't loaded, so this is my stop, I'll be getting off now.

I can never remember scala.reflect.runtime.ReflectionUtils.show.

Shout out to Paul for "and parent being..." in the text.

Error: unable to load class: t11802.SamplePloogin1 from loader: scala.tools.nsc.plugins.Plugins$$anon$1@6f001fad of type class scala.tools.nsc.plugins.Plugins$$anon$1 with classpath [file:/C:/Users/andre/Documents/scala/test/files/run/t11802-pluginsdir-run.obj/myplugins/p1.jar] and parent being jdk.internal.loader.ClassLoaders$AppClassLoader@7aec35a of type class jdk.internal.loader.ClassLoaders$AppClassLoader with classpath [<unknown>] and parent being jdk.internal.loader.ClassLoaders$PlatformClassLoader@112c1fc0 of type class jdk.internal.loader.ClassLoaders$PlatformClassLoader with classpath [<unknown>] and parent being primordial classloader with boot classpath [<unknown>]
error: Missing required plugin: ploogin1_1

@som-snytt
Copy link
Contributor Author

som-snytt commented Nov 21, 2019

Oh, the jar entries have backslashes.

I may have given the false impression that I hate windows with an ordinary loathing.

@som-snytt som-snytt force-pushed the issue/zip-broken-windows branch from e42a14d to 767bd9f Compare November 21, 2019 01:47
@som-snytt som-snytt changed the title Fix broken windows test with more closing Less yammering, more hammering: fix two broken windows tests Nov 21, 2019
@SethTisue
Copy link
Member

SethTisue commented Nov 21, 2019

queued: https://scala-ci.typesafe.com/job/scala-2.13.x-integrate-windows/1181/

@som-snytt
Copy link
Contributor Author

som-snytt commented Nov 21, 2019 •
edited
Loading

Yeah, baby.

Finished: SUCCESS

We should say, "All dots," to mean "SUCCESS", or no Xs:

# starting 1917 tests in run
Note: test execution will be non-parallel under -Dpartest.exec.in.process
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
.............................................

- How did it go today?
- All dots, baby.
Recognizing also:
All dots glitter is not gold.
We can rename the CI to:
Dots Entertainment!

@SethTisue
Copy link
Member

SethTisue commented Nov 21, 2019 •
edited
Loading

LGTM for merge, but @lrytz can you make the call about reflect.io? I have lingering doubt about whether it's okay to whitelist.

@SethTisue SethTisue added the internal not resulting in user-visible changes (build changes, tests, internal cleanups) label Nov 21, 2019
@som-snytt som-snytt mentioned this pull request Nov 21, 2019
Closed
@som-snytt
Copy link
Contributor Author

som-snytt commented Nov 22, 2019

@SethTisue I'll change it, it was just for the free resource release.

@som-snytt
Fix broken windows test with more closing
60c56a2 
Just closing didn't fix it on windows; another option
is opening for delete on close; or, as here, just
demur on windows because who cares.

Also fix another broken test on windows where a jar
file was created with backslash in the entry names.
@som-snytt som-snytt force-pushed the issue/zip-broken-windows branch from 767bd9f to 60c56a2 Compare November 23, 2019 16:14
@SethTisue SethTisue merged commit 16ef5eb into scala:2.13.x Nov 27, 2019
@SethTisue
Copy link
Member

SethTisue commented Nov 27, 2019

less slobbering, more clobbering! 👊

@SethTisue
Copy link
Member

SethTisue commented Nov 27, 2019

@som-snytt it appears from https://scala-ci.typesafe.com/view/scala-2.12.x/job/scala-2.12.x-integrate-windows/ (first failure: https://scala-ci.typesafe.com/view/scala-2.12.x/job/scala-2.12.x-integrate-windows/1558/) that the t11802 fix needs a 2.12.x backport, but leave it ’til you're done basking in https://twitter.com/adriaanm/status/1199678543317413888

@som-snytt
Copy link
Contributor Author

som-snytt commented Nov 28, 2019 •
edited
Loading

I can't keep the branches straight.

Thanks @adriaanm does that include reversions? Edit: it gives new meaning to the phrase, "elusive quality."

@som-snytt som-snytt deleted the issue/zip-broken-windows branch November 28, 2019 00:30
@som-snytt som-snytt mentioned this pull request Nov 29, 2019
Merged
@SethTisue
Copy link
Member

SethTisue commented Dec 5, 2019

type-tag-leak.scala has failed 2 out of the last 8 runs at https://scala-ci.typesafe.com/view/scala-2.12.x/job/scala-2.12.x-integrate-windows/

@som-snytt
Copy link
Contributor Author

som-snytt commented Dec 5, 2019

Is that the good news or the bad news?

@SethTisue
Copy link
Member

SethTisue commented Dec 9, 2019

re: type-tag-leak, I've alerted Jason and Lukas over at #8546

@lrytz lrytz mentioned this pull request Oct 6, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lrytz lrytz lrytz left review comments

Assignees

No one assigned

Labels

internal not resulting in user-visible changes (build changes, tests, internal cleanups)

Projects

None yet

Milestone

2.13.2

Development

Successfully merging this pull request may close these issues.

4 participants

@som-snytt @SethTisue @lrytz @scala-jenkins