Fix #3661: Load Linker Reflectively #3805

gzm0 · 2019-10-08T15:06:15Z

No description provided.

gzm0 · 2019-10-08T15:10:00Z

Note that fullOptJS does currently not work because there is a dependency problem on Guava between closure compiler and jimfs. As a result of the class loading, the older version gets loaded and there are binary incompatibilites.

gzm0 · 2019-10-10T16:10:49Z

Hah, now of course we have the problem that we load the wrong Scala version :)

Alternatives:

Fiddle with the build invocation until it works.
Make the interface plain Java (can we even do this in Scala)? Main problem would be futures AFAICS.

@sjrd WDYT?

sjrd

I'm confused by the fact that the sbt plugin still depends on the full linker, then loads the linker reflectively but from the same class loader. I was expecting something more along the lines of: the sbt plugin does not depend on the full linker, but rather downloads it separately from the classpath of the sbt plugin, then creates a class-loader to load the full linker inside it.

That would also solve things like #3193, and any other mismatch in dependencies on stuff that is also depended on by other sbt plugins.

To resolve the Guava thing and the Scala version thing, I think we need something similar to what we had done for the JettyClassLoader of PhantomJS in 0.6.x: a custom ClassLoader that loads java.*, scala.* and org.scalajs.linker.interface.* from its parent class-loader, but everything else is forced not to go the parent class-loader.

sjrd · 2019-10-11T09:18:02Z

sbt-plugin/src/main/scala/org/scalajs/sbtplugin/ScalaJSPlugin.scala

@@ -183,16 +202,21 @@ object ScalaJSPlugin extends AutoPlugin {

        scalaJSLinkerConfig := StandardConfig(),

+        scalaJSLinkerImpl := LinkerImpl.default(getClass().getClassLoader()),


What's the point of loading reflectively if you are using the current class loader?

gzm0 · 2019-10-11T15:11:27Z

downloads it separately from the classpath of the sbt plugin

That is of course an even nicer solution. Actually was considering it, but didn't because it needs a custom configuration :) I'll do this.

To resolve the Guava thing and the Scala version thing, I think we need something similar to what we had done for the JettyClassLoader of PhantomJS in 0.6.x

This will solve the Guava thing, but not the Scala version thing: The linker interface depends on the scala library. So we have to load at least these classes and their transitive dependencies from the parent classloader. Otherwise we'll have a class mismatch.

sjrd · 2019-10-11T15:18:49Z

That is of course an even nicer solution. Actually was considering it, but didn't because it needs a custom configuration :) I'll do this.

You don't need a custom configuration anymore with sbt 1.x. You can directly invoke the librarymanagement API. See here an example how to do that:
https://github.com/sjrd/sbt-dynscalajs/blob/06293c262b24463361d43d37130448b331c0987d/sbt-dynscalajs/src/main/scala/be/doeraene/sbtdynscalajs/DynScalaJSPlugin.scala#L256-L276

This will solve the Guava thing, but not the Scala version thing: The linker interface depends on the scala library. So we have to load at least these classes and their transitive dependencies from the parent classloader. Otherwise we'll have a class mismatch.

Hum yes but as long as the linker interface you load uses a 2.12.x Scala version, it will correctly link with the Scala 2.12.y used by sbt.

gzm0 · 2019-10-14T04:35:53Z

@SethTisue before I investigate the failures here further: Could you have a look at the changes to how we chose the Scala version in this build and check if it is community build compatible? If no, how do I have to adapt it?

SethTisue · 2019-10-16T04:03:51Z

offhand it looks okay to me, I'm not 100% sure, but we can always deal it with later

gzm0 · 2019-10-16T04:34:24Z

So the CI failures look like sbt is loading the wrong SJS compiler when compiling the Test configuration only. I can reproduce the errors locally: For a JS project, <project>/compile works fine, whereas <project>/test:compile does not.

gzm0

Self review.

gzm0 · 2019-10-17T05:08:01Z

project/Build.scala

@@ -495,7 +506,7 @@ object Build {
        if (v < 8)
          throw new MessageOnlyException("This build requires JDK 8 or later. Aborting.")
        v
-      }
+      },


Spurious change.

gzm0 · 2019-10-17T05:08:09Z

project/Build.scala

      organization := "org.scala-js",
+      scalaVersion := "2.12.8",


Spurious change

gzm0 · 2019-10-17T05:09:04Z

sbt-plugin/src/main/scala/org/scalajs/sbtplugin/ScalaJSPlugin.scala

@@ -57,13 +57,17 @@ object ScalaJSPlugin extends AutoPlugin {

    // All our public-facing keys


Should we make the internal keys private?

No, I'd rather not. In the end all keys are public in the sbt shell. Making them private in the code is a fallacy.

gzm0 · 2019-10-17T05:09:27Z

sbt-plugin/src/main/scala/org/scalajs/sbtplugin/ScalaJSPlugin.scala

+        KeyRanks.Invisible)
+
+    val scalaJSLinkerImpl = TaskKey[LinkerImpl]("scalaJSLinkerImpl",
+        "Factory for a Scala.js linker", KeyRanks.Invisible)


TODO: Describe this.

gzm0 · 2019-10-17T05:09:40Z

sbt-plugin/src/main/scala/org/scalajs/sbtplugin/ScalaJSPlugin.scala

+        "Factory for a Scala.js linker", KeyRanks.Invisible)
+
+    val scalaJSLinkerBox = SettingKey[CacheBox[ClearableLinker]]("scalaJSLinkerBox",
+        "CacheBox for a Scala.js linker", KeyRanks.Invisible)


Mark this as internal.

gzm0 · 2019-10-17T05:10:23Z

sbt-plugin/src/main/scala/org/scalajs/sbtplugin/ScalaJSPlugin.scala

+      globalIRCacheBox.foreach { globalIRCache =>
+        logger.debug("Global IR cache stats: " + globalIRCache.stats.logLine)
+      }
+    }


I really don't like this. Can we find a better way to do this? Ideally after every task that users the linker :)

gzm0 · 2019-10-17T05:13:29Z

sbt-plugin/src/main/scala/org/scalajs/sbtplugin/ScalaJSPlugin.scala


          { () =>
            prev()
            ScalaJSPluginInternal.closeAllTestAdapters()
-            ScalaJSPluginInternal.globalIRCache.clearStats()
+            globalIRCacheBox.foreach(_.clearStats())


Would it be acceptable to log IR cache stats just here? That would get rid of the strange logger and also make sure nobody ever forgets it. At the cost of only logging once if multiple linking (or other IR consuming) operations are executed in a single swoop.

I think that would be acceptable, yes.

sbt-plugin/src/main/scala/org/scalajs/sbtplugin/ScalaJSPlugin.scala

This is so that the main sbt-plugin-test does not have a direct dependency on the Linker anymore (see scala-js#3805 for details).

gzm0 · 2019-10-20T16:22:45Z

Updated.

sjrd

LGTM. Should we merge this now or wait until we've figured out the community build?

gzm0 · 2019-10-21T07:38:47Z

Should we merge this now or wait until we've figured out the community build?

I guess that depends on @SethTisue's confidence level that we can make it work in this form? Personally I'm OK with waiting.

gzm0 · 2019-10-22T15:25:49Z

It seems that #3817 is likely to change community build integration as well :S So maybe we should just move forward with this. @sjrd WDYT?

sjrd · 2019-10-22T15:36:01Z

As I wrote in #3817, I think the changes there won't qualitatively change the challenge with the community build. So if we can validate the community build integration for this PR, the solution will transfer to #3817.

gzm0 · 2019-10-22T17:23:52Z

As I wrote in #3817, I think the changes there won't qualitatively change the challenge with the community build. So if we can validate the community build integration for this PR, the solution will transfer to #3817.

Right. In fact I realize that our solution to this does not transfer to #3817: We will need two levels of pinning there: The linker needs to be conditionally pinned; the linker private library needs to be always pinned.

Another option is to create a project per major scala version for all projects. It seems that would solve the version problem for us for all cases (including using scripted with different scala versions). Minor versions could be selected with ++v.

WDYT?

SethTisue · 2019-10-22T18:47:45Z

(I am committed to helping find a way forward here, but I'm on semi-vacation for a while as I move from CA to NV, so my time is a bit scarce right now)

sjrd · 2019-10-22T18:59:46Z

Using one project per major Scala version for all projects seems reasonable, indeed.

This is much simpler and good enough. It also will not force us to expose a strange setting in the next commit.

This gives us the following advantages: - Allows us to give resources to the linker that the build itself compiles first (unblocks scala-js#3537). - Hides linker dependencies away from other sbt plugins. Notably the Google Closure Compiler and Guava (fixes scala-js#3193).

SethTisue · 2019-11-13T17:47:41Z

this is green in the 2.12 community build since the Scala major version happens to align between the build and the meta-build

but in the 2.11 and 2.13 builds, it fails, since the changes here pull in linker.v2_12 but then dbuild overrides linker.v2_12's Scala version to 2.1{1,3}

I have some ideas about how to work around it. I'll mess with it

SethTisue · 2019-11-13T18:15:16Z

it's best if I don't have to fork your build, since then my fork would always be falling out of date.

could you offer me a setting I could flip that would cause scalaJSLinkerImpl to be set differently in the community build context — namely, by not doing this problematic fullClasspath in (Build.linker.v2_12, Runtime thing, but instead retrieving the 2.12 artifact from Maven Central (how? above, Seb suggested "You can directly invoke the librarymanagement API") and using that. does that seem plausible?

sjrd · 2019-11-13T18:29:36Z

The artifacts you'll find on Maven Central will not correspond to the version being tested, so that's not a viable path forward I'm afraid.

I see two possible paths:

The easiest I think would be to tell dbuild not to mess with some projects in the build, in this case any of the 2_12 projects. That includes not setting the Scala version, and not overriding dependencies.
Otherwise, maybe we can tell dbuild to first build normally and publish locally the relevant 2.12 projects, then go ahead with its normal patching everything behavior, while using scalaJSLinkerImpl to load from the locally published version.

SethTisue · 2019-11-13T18:35:39Z

I've opened a separate ticket, scala/community-build#1009, about whether it makes sense to continue using dbuild for Scala.js at all. let's also discuss there

SethTisue · 2019-11-13T18:38:19Z

I see two possible paths:

I was thinking along these lines as well, but it doesn't seem easy to me, I made a few stabs at it already, my stab like your first suggestion didn't work, and in my stab like your second suggestion I got bogged down and it seemed like a time sink without success being assured. so I'd like to discuss on #1009 a bit first, then depending on how that goes, perhaps return to this ticket to go into further details about the stick-with-dbuild option.

gzm0 force-pushed the load-reflectively branch from fc5161d to 52b2b0b Compare October 8, 2019 15:08

gzm0 force-pushed the load-reflectively branch 2 times, most recently from 14efa2a to 10ca987 Compare October 10, 2019 15:25

gzm0 changed the title ~~WIP Fix #3661: Load Linker Reflectively~~ Fix #3661: Load Linker Reflectively Oct 10, 2019

gzm0 marked this pull request as ready for review October 10, 2019 15:31

gzm0 requested a review from sjrd October 10, 2019 15:31

sjrd reviewed Oct 11, 2019

View reviewed changes

gzm0 force-pushed the load-reflectively branch 2 times, most recently from 8e66c55 to b7772c9 Compare October 13, 2019 15:09

gzm0 force-pushed the load-reflectively branch 2 times, most recently from a72b00a to e66371c Compare October 15, 2019 15:48

gzm0 force-pushed the load-reflectively branch 5 times, most recently from 0129372 to d338ecc Compare October 17, 2019 04:41

gzm0 commented Oct 17, 2019

View reviewed changes

gzm0 force-pushed the load-reflectively branch 4 times, most recently from 8da8c9f to 1918e78 Compare October 18, 2019 08:13

gzm0 commented Oct 18, 2019

View reviewed changes

sbt-plugin/src/main/scala/org/scalajs/sbtplugin/ScalaJSPlugin.scala Show resolved Hide resolved

gzm0 force-pushed the load-reflectively branch from 1918e78 to 538a317 Compare October 18, 2019 11:44

gzm0 added a commit to gzm0/scala-js that referenced this pull request Oct 19, 2019

Use scripted for concurrent linker test

3d3e149

This is so that the main sbt-plugin-test does not have a direct dependency on the Linker anymore (see scala-js#3805 for details).

gzm0 force-pushed the load-reflectively branch from 9ce8ade to 586d011 Compare October 20, 2019 16:17

sjrd approved these changes Oct 20, 2019

View reviewed changes

sjrd mentioned this pull request Oct 22, 2019

Include RuntimeLong.sjsir as a resource of the linker. #3817

Merged

gzm0 force-pushed the load-reflectively branch from 586d011 to 7b6852f Compare November 10, 2019 14:01

gzm0 changed the title ~~Fix #3661: Load Linker Reflectively~~ DO NOT MERGE Fix #3661: Load Linker Reflectively Nov 10, 2019

gzm0 force-pushed the load-reflectively branch 2 times, most recently from 823c616 to bea0a85 Compare November 11, 2019 16:09

Log IRCache stats once at the end of each run

59740fb

This is much simpler and good enough. It also will not force us to expose a strange setting in the next commit.

gzm0 force-pushed the load-reflectively branch from bea0a85 to df3916a Compare November 12, 2019 22:10

gzm0 changed the title ~~DO NOT MERGE Fix #3661: Load Linker Reflectively~~ Fix #3661: Load Linker Reflectively Nov 12, 2019

gzm0 force-pushed the load-reflectively branch from df3916a to 341bc00 Compare November 12, 2019 22:17

sjrd approved these changes Nov 12, 2019

View reviewed changes

gzm0 merged commit 2437b45 into scala-js:master Nov 13, 2019

gzm0 deleted the load-reflectively branch November 13, 2019 05:08

SethTisue mentioned this pull request Nov 13, 2019

retain Scala.js in the community build? is it worth the effort? scala/community-build#1009

Closed

sjrd mentioned this pull request Jan 22, 2020

WIP: Update CI cache to include Scala.js artifacts scala/scala3#8071

Merged

		@@ -183,16 +202,21 @@ object ScalaJSPlugin extends AutoPlugin {

		scalaJSLinkerConfig := StandardConfig(),

		scalaJSLinkerImpl := LinkerImpl.default(getClass().getClassLoader()),

		@@ -57,13 +57,17 @@ object ScalaJSPlugin extends AutoPlugin {

		// All our public-facing keys

Fix #3661: Load Linker Reflectively #3805

Fix #3661: Load Linker Reflectively #3805

Uh oh!

Conversation

gzm0 commented Oct 8, 2019

Uh oh!

gzm0 commented Oct 8, 2019

Uh oh!

gzm0 commented Oct 10, 2019

Uh oh!

sjrd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gzm0 commented Oct 11, 2019

Uh oh!

sjrd commented Oct 11, 2019

Uh oh!

gzm0 commented Oct 14, 2019

Uh oh!

SethTisue commented Oct 16, 2019

Uh oh!

gzm0 commented Oct 16, 2019

Uh oh!

gzm0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjrd Oct 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gzm0 commented Oct 20, 2019

Uh oh!

sjrd left a comment

Choose a reason for hiding this comment

Uh oh!

gzm0 commented Oct 21, 2019

Uh oh!

gzm0 commented Oct 22, 2019

Uh oh!

sjrd commented Oct 22, 2019

Uh oh!

gzm0 commented Oct 22, 2019

Uh oh!

SethTisue commented Oct 22, 2019

Uh oh!

sjrd commented Oct 22, 2019

Uh oh!

SethTisue commented Nov 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SethTisue commented Nov 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sjrd commented Nov 13, 2019

Uh oh!

SethTisue commented Nov 13, 2019

Uh oh!

SethTisue commented Nov 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

sjrd Oct 17, 2019 •

edited

Loading

SethTisue commented Nov 13, 2019 •

edited

Loading

SethTisue commented Nov 13, 2019 •

edited

Loading

SethTisue commented Nov 13, 2019 •

edited

Loading