Skip to content
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

Remove scala2-library-tasty-tests project #19379

Merged
merged 2 commits into from
Mar 19, 2024
Merged

Remove scala2-library-tasty-tests project #19379

merged 2 commits into from
Mar 19, 2024

Conversation

nicolasstucki
Copy link
Contributor

@nicolasstucki nicolasstucki commented Jan 5, 2024 •
edited
Loading

Split BootstrappedStdLibTASYyTest into the part that tests the TASTy inspector and the part that tests recompilation.

The tests/run/scala2-library-test test will be tested with the original Scala 2 library JAR and with the Scala 2 library TASTy JAR depending on the scala2Library SBT setting.

We also remove scala2-library-tasty-tests as it is now empty and will not serve any purpose anymore.

This test also discovered a bug in TASTYRun with the path separators of JarArchives in Windows. The "/" is used in the JAR paths but the "" is used for OS paths. This is now fixed.

[test_java8]

@nicolasstucki nicolasstucki changed the title Port tests form scala2-library-tasty-tests to normal tests Remove scala2-library-tasty-tests project Jan 5, 2024
@nicolasstucki nicolasstucki force-pushed the simplify-tests-from-scala2-library-tasty-tests branch 2 times, most recently from d12ebe4 to 7f61456 Compare January 8, 2024 10:20
@nicolasstucki nicolasstucki force-pushed the simplify-tests-from-scala2-library-tasty-tests branch from 7f61456 to adbdcf0 Compare January 31, 2024 12:58
@nicolasstucki nicolasstucki self-assigned this Feb 1, 2024
@nicolasstucki nicolasstucki force-pushed the simplify-tests-from-scala2-library-tasty-tests branch from adbdcf0 to 30403cf Compare February 7, 2024 15:00
@nicolasstucki
Copy link
Contributor Author

tests/run-with-compiler/scala2-library-from-tasty.scala is timing out on CI. It does not loacally. Maybe we can split this test into smaller parts.

@nicolasstucki nicolasstucki force-pushed the simplify-tests-from-scala2-library-tasty-tests branch 2 times, most recently from c1fd0d8 to f33065d Compare February 12, 2024 09:51
@nicolasstucki
Copy link
Contributor Author

I had to make sure that the output of the from tasty tests are set explicitly with -d. Otherwise, it generated the classes in the root directory.

@nicolasstucki nicolasstucki force-pushed the simplify-tests-from-scala2-library-tasty-tests branch from f33065d to 0d5a0ad Compare March 4, 2024 14:59
nicolasstucki
nicolasstucki commented Mar 4, 2024
View reviewed changes
tests/run-with-compiler/scala2-library-from-tasty.scala Outdated
val args = Array(
"-classpath", ClasspathFromClassloader(getClass.getClassLoader),
"-from-tasty",
"-d", "out/scala2-library-from-tasty-test-output.jar",
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 added this line to this test.

tests/run-with-compiler/scala2-library-from-tasty-jar.scala Outdated
val args = Array(
"-classpath", ClasspathFromClassloader(getClass.getClassLoader),
"-from-tasty",
"-d", "out/scala2-library-from-tasty-jar-test-output.jar",
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 added this line to this test.

@nicolasstucki nicolasstucki marked this pull request as ready for review March 4, 2024 15:02
@nicolasstucki nicolasstucki force-pushed the simplify-tests-from-scala2-library-tasty-tests branch 5 times, most recently from 2e726b9 to 04e5df7 Compare March 7, 2024 16:22
@nicolasstucki
Port tasty inspector test from scala2-library-tasty-tests
1e56ca0 
Part of scala#19379

The new test is run on all available configurations. Now it also runs on
Windows.
@nicolasstucki nicolasstucki mentioned this pull request Mar 8, 2024
Closed
@nicolasstucki
Copy link
Contributor Author

When testing from TASTy on Windows we currently get the following errors

Test 'tests\run-with-compiler\scala2-library-from-tasty-jar.scala' failed with output:
class /scala/volatile cannot be unpickled because no class file was found for denot: val <none>
class /scala/unchecked cannot be unpickled because no class file was found for denot: val <none>
class /scala/transient cannot be unpickled because no class file was found for denot: val <none>
class /scala/throws cannot be unpickled because no class file was found for denot: val <none>
...

I assume that the bug is cause by the extra / at the start of the class name. Probably an issue handling \ separators.

@nicolasstucki nicolasstucki force-pushed the simplify-tests-from-scala2-library-tasty-tests branch 2 times, most recently from 77ad190 to c291cd3 Compare March 11, 2024 12:22
@nicolasstucki nicolasstucki force-pushed the simplify-tests-from-scala2-library-tasty-tests branch 3 times, most recently from d62c4b7 to 47fe9f8 Compare March 19, 2024 10:53
@nicolasstucki
Port tests form scala2-library-tasty-tests to normal tests
96b57b8 
Split BootstrappedStdLibTASYyTest into the part that tests the TASTy
inspector and the part that tests recompilation.

We also remove `scala2-library-tasty-tests` as it is now empty and will
not serve any purpose anymore.

This test also discovered a bug in TASTYRun with the path separators of
`JarArchive`s in Windows. The "/" is used in the JAR paths but the "\"
is used for OS paths. This is now fixed.
@nicolasstucki nicolasstucki force-pushed the simplify-tests-from-scala2-library-tasty-tests branch from 47fe9f8 to 96b57b8 Compare March 19, 2024 10:54
nicolasstucki
nicolasstucki commented Mar 19, 2024
View reviewed changes
tests/run-with-compiler/scala2-library-from-tasty-jar.scala
blacklistsOnlyContainsClassesThatExist()
// FIXME this test does not work on JDK8
// Caused by: dotty.tools.dotc.core.TypeError$$anon$1: package scala.quoted.runtime.Expr does not have a member method quote
if System.getProperty("java.specification.version") != "1.8" then
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a possible bug when running the from-TASTy compiler on JDK8 or with the test itself. I will open an issue to investigate this further.

Note that the Windows tests found a bug in TASTYRun.scala that was fixed but then failed due it running on JDK8. Therefore this fix will not be tested until we fix this issue or run Windows tests on a newer version of the JDK.

bishabosha
bishabosha reviewed Mar 19, 2024
View reviewed changes
compiler/src/dotty/tools/dotc/fromtasty/TASTYRun.scala
.filter(e => Path.extension(e) == "tasty" && !fromTastyIgnoreList(e))
.map(e => e.stripSuffix(".tasty").replace(File.separator, "."))
.map(_.stripPrefix("/")) // change paths from absolute to relative
.filter(e => Path.extension(e) == "tasty" && !fromTastyIgnoreList(e.replace("/", File.separator)))
Copy link
Member

@bishabosha bishabosha Mar 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

seems ok, but perhaps fromTastyIgnoreList should accept binary names (. separator, no file extension) rather than paths?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe. Out of scope for this PR.

bishabosha
bishabosha reviewed Mar 19, 2024
View reviewed changes
tests/run-with-compiler/scala2-library-from-tasty.scala
// at dotty.tools.backend.jvm.BCodeHelpers$BCInnerClassGen.getClassBTypeAndRegisterInnerClass$(BCodeHelpers.scala:210)
// at dotty.tools.backend.jvm.BCodeSkelBuilder$PlainSkelBuilder.getClassBTypeAndRegisterInnerClass(BCodeSkelBuilder.scala:62)
// at dotty.tools.backend.jvm.BCodeHelpers$BCInnerClassGen.internalName(BCodeHelpers.scala:237)
s"scala${separator}Array.tasty",
Copy link
Member

Choose a reason for hiding this comment

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

see my comment above that perhaps -Yfrom-tasty-ignore-list should be a list of binary names, rather than relative paths

@nicolasstucki nicolasstucki merged commit 01c6623 into scala:main Mar 19, 2024
19 checks passed
@nicolasstucki nicolasstucki deleted the simplify-tests-from-scala2-library-tasty-tests branch March 19, 2024 14:06
@Kordyjan Kordyjan added this to the 3.4.2 milestone Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bishabosha bishabosha bishabosha approved these changes

Assignees

@bishabosha bishabosha

Labels
None yet
Projects
None yet
Milestone
3.4.2
Development

Successfully merging this pull request may close these issues.
3 participants
@nicolasstucki @bishabosha @Kordyjan