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.

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

add more scalafix projects #1673

Closed
SethTisue opened this issue Jun 13, 2023 · 12 comments
Closed

add more scalafix projects #1673

SethTisue opened this issue Jun 13, 2023 · 12 comments
Assignees

Comments

@SethTisue
Copy link
Member

SethTisue commented Jun 13, 2023

as per scalacenter/scalafix#1699 (previous history: #1602), -release:8 should allow code that references sun.misc.Unsafe to compile. I'll need to dig into why it isn't working in the dbuild context

I'm also having a weird Scala version thing where the project extracts with version=2.13.11 but there's an error message (that I don't have handy) when the nightly version number is used

@bjaglin
Copy link

bjaglin commented Jun 13, 2023

as per scalacenter/scalafix#1699 (previous history: #1602), -release:8 should allow code that references sun.misc.Unsafe to compile. I'll need to dig into why it isn't working in the dbuild context

Actually, the current behavior of -release:8 does prevent usage of sun.misc.Unsafe, but sbt's rt.jar effectively cancels this behavior (as discussed in sbt/sbt#6817). So it might just be that the dbuild context is stricter?

@bjaglin
Copy link

bjaglin commented Jun 13, 2023

I'm also having a weird Scala version thing where the project extracts with version=2.13.11 but there's an error message (that I don't have handy) when the nightly version number is used

Let me know if/how I can help. I will try to get dbuild running locally this week, understanding how the scala/scalameta versions get injected specifically, evaluating their impact through projects using scalafix-testkit .

I realized that I never followed up on your previous request, apologies for that. Now that I have a bit more free time to invest on OSS, I definitely want to help on that front, to continue detecting regressions like scalacenter/scalafix#1556 💪

@SethTisue
Copy link
Member Author

@bjaglin I'm very glad you're interested in helping — but note that I have so much experience troubleshooting dbuild issues that the most efficient approach is probably for me to be the next one to dig, and then let you know if I have questions

@SethTisue
Copy link
Member Author

SethTisue commented Jun 14, 2023

you are right — it's -release:8 that prevents it from compiling. on JDK 17 this succeeds:

scala-cli compile -S 2.13.11 \
  --dep org.scalameta::scalameta:4.7.8 \
  scalafix/scalafix-reflect/src/main/scala/scalafix/internal/reflect/ClasspathOps.scala

but if I add -O -release:8 it fails with

[error] not found: object sun
[error] import sun.misc.Unsafe
[error]        ^^^

this is the opposite of what I assumed.

@SethTisue
Copy link
Member Author

okay, the sun.misc.Unsafe thing is sorted, it just needed:

+    "removeScalacOptions -release 8"

@SethTisue
Copy link
Member Author

as for the thing where it doesn't work with a 2.13.12 nightly but does work with version=2.13.11 ./run scalafix, the problem is reproducible outside of dbuild by simply starting sbt in the scalafix repo and typing ++2.13.12-bin-1a2373b!:

% sbt
[info] welcome to sbt 1.8.3 (Eclipse Adoptium Java 17.0.7)
...
sbt:scalafix> ++2.13.12-bin-1a2373b!
[info] Forcing Scala version to 2.13.12-bin-1a2373b on all projects.
[info] Reapplying settings...
java.lang.RuntimeException: project matching List(PlatformAxis(jvm,JVM,jvm)) and 2.13.12-bin-1a2373b was not found
	at scala.sys.package$.error(package.scala:30)
	at sbt.internal.ProjectMatrix$ProjectMatrixDef$AxisBaseProjectFinder.$anonfun$apply$1(ProjectMatrix.scala:466)
	at scala.Option.getOrElse(Option.scala:189)
	at sbt.internal.ProjectMatrix$ProjectMatrixDef$AxisBaseProjectFinder.apply(ProjectMatrix.scala:466)
	at ScalafixBuild$autoImport$.$anonfun$resolve$1(ScalafixBuild.scala:134)
	at sbt.internal.util.EvaluateSettings$MixedNode.evaluate0(INode.scala:228)
	at sbt.internal.util.EvaluateSettings$INode.evaluate(INode.scala:170)
	at sbt.internal.util.EvaluateSettings.$anonfun$submitEvaluate$1(INode.scala:87)
	at sbt.internal.util.EvaluateSettings.sbt$internal$util$EvaluateSettings$$run0(INode.scala:99)
	at sbt.internal.util.EvaluateSettings$$anon$3.run(INode.scala:94)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
	at java.base/java.lang.Thread.run(Thread.java:833)

it looks like this has to do with some custom code in the Scalafix build; at ScalafixBuild.scala:134 we see

        val project = matrix.jvm(sv)
        Def.setting((project / key).value)                                                          

@bjaglin if you want to try this yourself outside of dbuild and make it possible to do ++2.13.12-bin-1a2373b!, note that you would probably need to publish Scalameta locally for that specific Scala version in order to actually compile or run anything. but perhaps it's not necessary for you to get that deep into it; do you see a way to at least allow the build to load?

SethTisue added a commit to SethTisue/community-build that referenced this issue Jun 14, 2023
@SethTisue
Copy link
Member Author

SethTisue commented Jun 14, 2023

oh, just getting the build to load is easy; editing project/Dependencies.scala as follows does the trick:

-  val scala213 = "2.13.11"
+  val scala213 = "2.13.12-bin-1a2373b"

seems to do the trick

I'm trying out a change of the form

-  val scala213 = "2.13.11"
+  val scala213 = sys.props.getOrElse("scala.nightly", "2.13.11")

since I can control system properties from dbuild

SethTisue added a commit to SethTisue/community-build that referenced this issue Jun 14, 2023
@SethTisue
Copy link
Member Author

SethTisue commented Jun 14, 2023

I merged #1674 which gains us some good ground that I'm pleased with.

There's two areas in which further progress might be possible:

One: of the five repos which use scalafix-testkit, #1674 was only able to enable testing in one. The other four seem to be on too-old Scalafix versions. It might be nice to get those repos on a newer version, to increase the overall amount of testing Scalafix gets in the community build. (Or maybe you or me attempting that would be an out-of-scope fishing expedition...)

Two: you'll see in #1674 that I'm still excluding some tests, namely "ScalafixSuite.scala" || "ScalafixImplSuite.scala" || "ScalafixArgumentsSuite.scala". I might be able to make them work in the dbuild context with some digging. But I don't have time right now.

@SethTisue
Copy link
Member Author

SethTisue commented Jun 14, 2023

Note that in #1674 I forked Scalafix in order to include this change: scalacommunitybuild/scalafix@df9636f

@bjaglin would you take the change if I PRed it? Or can you think of a better way to make it work with Scala nightlies?

@SethTisue
Copy link
Member Author

@bjaglin ping

@SethTisue
Copy link
Member Author

@bjaglin I added this to #1697 and it worked in a local test, so we're good, thanks!

leaving the ticket open since enabling more subprojects remains an open possibility

@SethTisue
Copy link
Member Author

I think I was confused in my previous comment. We have:

  extra.projects: ["*2_13"]
  extra.exclude: ["docs2_13"]

so that's good, and yes we are still excluding some tests but we normally track that kind of with comments in the .conf file, rather than keeping tickets open. so, closing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants