Skip to content

Cut dependencies on future modules #5830

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

Merged
merged 6 commits into from
Apr 7, 2017
Merged

Cut dependencies on future modules #5830

merged 6 commits into from
Apr 7, 2017

Conversation

lrytz
Copy link
Member

@lrytz lrytz commented Apr 5, 2017

Re-submission of #5677

@scala-jenkins scala-jenkins added this to the 2.13.0-M2 milestone Apr 5, 2017
@lrytz
Copy link
Member Author

lrytz commented Apr 5, 2017

My changes compared to #5677

  • Removed new deprecations
  • Rebased

Note that 6411170 recently added a new method scala.util.Properties.coloredOutputEnabled, which I left untouched.

@lrytz
Copy link
Member Author

lrytz commented Apr 5, 2017

The remaining test failures look like this:

[error] Test tools.test.osgi.reflection.basic.BasicReflectionTest.basicMirrorThroughOsgi failed
        scala.reflect.internal.MissingRequirementError: package object scala.sys not found.
[error]     at scala.reflect.internal.MissingRequirementError$.signal(MissingRequirementError.scala:17)
[error]     at scala.reflect.internal.MissingRequirementError$.notFound(MissingRequirementError.scala:18)
[error]     at scala.reflect.internal.Mirrors$RootsBase.getPackageObject(Mirrors.scala:184)
[error]     at scala.reflect.internal.Mirrors$RootsBase.getPackageObject(Mirrors.scala:180)
[error]     at scala.reflect.internal.Definitions$DefinitionsClass.SysPackage$lzycompute(Definitions.scala:339)
[error]     at scala.reflect.internal.Definitions$DefinitionsClass.SysPackage(Definitions.scala:339)
[error]     at scala.reflect.runtime.JavaUniverseForce.force(JavaUniverseForce.scala:244)
[error]     at scala.reflect.runtime.JavaUniverseForce.force$(JavaUniverseForce.scala:6)
[error]     at scala.reflect.runtime.JavaUniverse.force(JavaUniverse.scala:16)
[error]     at scala.reflect.runtime.JavaUniverse.init(JavaUniverse.scala:147)
[error]     at scala.reflect.runtime.JavaUniverse.<init>(JavaUniverse.scala:78)
[error]     at scala.reflect.runtime.package$.universe$lzycompute(package.scala:17)
[error]     at scala.reflect.runtime.package$.universe(package.scala:17)
[error]     at tools.test.osgi.reflection.basic.BasicReflectionTest.basicMirrorThroughOsgi(BasicReflection.scala:56)

The osgi tests all pass if this access to sys.props (removed in this PR) stays in SymbolTable:

--- a/src/reflect/scala/reflect/internal/SymbolTable.scala
+++ b/src/reflect/scala/reflect/internal/SymbolTable.scala
@@ -138,7 +138,8 @@ abstract class SymbolTable extends macros.Universe

   /** Dump each symbol to stdout after shutdown.
    */
-  final val traceSymbolActivity = System.getProperty("scalac.debug.syms") != null
+  final val traceSymbolActivity = sys.props contains "scalac.debug.syms"
   object traceSymbols extends {
     val global: SymbolTable.this.type = SymbolTable.this
   } with util.TraceSymbolActivity

@lrytz
Copy link
Member Author

lrytz commented Apr 5, 2017

I'm looking into this one, I think I'm getting somewhere.

@adriaanm
Copy link
Contributor

adriaanm commented Apr 5, 2017

Thanks! Re: osgi, does this have something to do with osgi enforcing boundaries and somehow the dependency on scala.sys was inferred and not explicitly specified?

@lrytz
Copy link
Member Author

lrytz commented Apr 5, 2017

No, I don't think it has to do with osgi itself, but with the classloader type used in runtime reflection. It seems to be specific to package objects. I'll figure it out I think.

@adriaanm
Copy link
Contributor

adriaanm commented Apr 5, 2017

I'll figure it out I think.

Na klar!

@lrytz
Copy link
Member Author

lrytz commented Apr 6, 2017

Finally figured it out... Your thorough work removed each and every dependency from scala-reflect to scala.sys, which makes the scala.sys package completely disappear from the META-INF/MANIFEST.MF in scala-reflect.jar. Adding any reference to scala.sys within the reflect codebase fixes it. For example i added class FlupediFlup { val p = scala.sys.Prop } to scala.reflect.io, then the manifests gets

...
Export-Package: ...
  scala.reflect.io ...uses:=" ... ,scala.sys"
...
Import-Package: ..., scala.sys ...

@lrytz
Copy link
Member Author

lrytz commented Apr 7, 2017

Now this is really weird: the MainGenericRunner (which calls System.exit) no longer waits for non-daemon threads to finish after this PR.. I haven't found the reason yet.

$ cat Test.scala
object Test {
  def main(args: Array[String]) {
    new Thread(new Runnable {
      def run = { Thread.sleep(1000); println("hi") }
    }).start
  }
}
$ scalac12 Test.scala
$ time scala12 Test.scala
hi
/Users/luc/scala/scala-2.12/bin/scala Test.scala  0.77s user 0.09s system 42% cpu 2.042 total
$ time ../build/quick/bin/scala Test
../build/quick/bin/scala Test  0.68s user 0.13s system 103% cpu 0.780 total
$

[edit]: not so weird after all, see comment below: #5830 (review)

@@ -101,5 +101,5 @@ class MainGenericRunner {
}

object MainGenericRunner extends MainGenericRunner {
def main(args: Array[String]): Unit = if (!process(args)) sys.exit(1)
def main(args: Array[String]): Unit = System.exit(if (process(args)) 0 else 1)
Copy link
Member Author

Choose a reason for hiding this comment

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

This changes behavior.. if process is true, we should not call exit. Otherwise this will kill the application without waiting for spawned non-daemon threads to finish. https://github.com/scala/scala/blob/v2.12.1/test/files/run/lazy-concurrent.scala is an example.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ouch! My bad :-/

@lrytz
Copy link
Member Author

lrytz commented Apr 7, 2017

Green now!

@lrytz
Copy link
Member Author

lrytz commented Apr 7, 2017

I'll squash some of the smaller commits into one to make them fewer.

@lrytz lrytz changed the title Cut dependencies on future modules [ci: last-only] Cut dependencies on future modules Apr 7, 2017
lrytz and others added 6 commits April 7, 2017 13:31
Since we no longer want to depend on `sys.error`, the compiler
also shouldn't generate calls to it.

This also fixes the OSGI tests. The failure was interesting: once
there were no more refernces to `scala.sys` left in scala-reflect,
the dependency on that package disappeared from scala-reflect.jar's
MANIFEST.MF. When instantiating scala.reflect.runtime.universe through
an osgi classloader, `getPackageObject("scala.sys")` in definitions
failed becasue the package could not be found through that classloader.
`scala.concurrent`, `scala.Console`, `scala.compat._`,
`sys.runtime`, `sys.addShutdownHook`, `sys.allThreads`,
`sys.env`.

Also fixes a test reducing references to static modules (Console, Predef)
delays initialization of object `NoStackTrace`. In `t2318`, this may now
happen *after* the security manager is installed. Therefore the security
manager needs to allow reading the system property
`scala.control.noTraceSuppression`
Use `throw new RuntimeException` (or a subclass) instead.
@lrytz lrytz merged commit 87c8eb7 into scala:2.13.x Apr 7, 2017
sjrd added a commit to sjrd/scala-js that referenced this pull request Apr 9, 2017
In scala/scala#5830, the definition of
`Sys_error` was removed from `Definitions.scala`. This commit
reintroduces it in our own codebase, as a quickfix to scala-js#2880.

This will need to be revisited as part of scala-js#2876 not use `sys.error`
at all, but that is less urgent.
sjrd added a commit to sjrd/scala-js that referenced this pull request Apr 9, 2017
In scala/scala#5830, the definition of
`Sys_error` was removed from `Definitions.scala`. This commit
reintroduces it in our own codebase, as a quickfix to scala-js#2880.

This will need to be revisited as part of scala-js#2876 not use `sys.error`
at all, but that is less urgent.
sjrd added a commit to sjrd/scala-js that referenced this pull request Apr 9, 2017
In scala/scala#5830, the definition of
`Sys_error` was removed from `Definitions.scala`. This commit
reintroduces it in our own codebase, as a quickfix to scala-js#2880.

This will need to be revisited as part of scala-js#2876 not use `sys.error`
at all, but that is less urgent.
@lrytz lrytz deleted the cut-deps branch April 12, 2017 07:50
@SethTisue SethTisue added the release-notes worth highlighting in next release notes label Apr 25, 2017
@som-snytt som-snytt mentioned this pull request Jan 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes worth highlighting in next release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants