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

tests are done against scala-xml dependency and not scala-xml local code !!! #20

Closed
dacr opened this issue Feb 28, 2014 · 15 comments
Closed

Comments

@dacr
Copy link
Contributor

dacr commented Feb 28, 2014

SBT_OPTS="-verbose"
sbt test

and you'll see for example that XMLEventReader used class is coming from .ivy2/cache/org.scala-lang.modules/scala-xml_2.11.0-M8/bundles/scala-xml_2.11.0-M8-1.0.0-RC7.jar and not from the locale one I'm testing and which was freshly recompiled ./target/scala-2.11.0-M8/classes/scala/xml/pull/XMLEventReader.class with fixes I'm trying to test.

All tests are concerned.

[Loaded scala.xml.pull.XMLEventReaderTest from file:/home/work/experiments/scala/scala-xml-dacr/target/scala-2.11.0-M8/test-classes/]
[Loaded scala.xml.pull.XMLEventReader from file:/home/work/.ivy2/cache/org.scala-lang.modules/scala-xml_2.11.0-M8/bundles/scala-xml_2.11.0-M8-1.0.0-RC7.jar]
[Loaded scala.xml.pull.XMLEventReader$POISON$ from file:/home/work/.ivy2/cache/org.scala-lang.modules/scala-xml_2.11.0-M8/bundles/scala-xml_2.11.0-M8-1.0.0-RC7.jar]
[Loaded scala.xml.pull.XMLEventReader$Parser from file:/home/work/.ivy2/cache/org.scala-lang.modules/scala-xml_2.11.0-M8/bundles/scala-xml_2.11.0-M8-1.0.0-RC7.jar]
[Loaded scala.xml.pull.XMLEventReader$Parser$$anonfun$run$1 from file:/home/work/.ivy2/cache/org.scala-lang.modules/scala-xml_2.11.0-M8/bundles/scala-xml_2.11.0-M8-1.0.0-RC7.jar]
[Loaded scala.xml.pull.XMLEventReader$Parser$$anonfun$setEvent$1 from file:/home/work/.ivy2/cache/org.scala-lang.modules/scala-xml_2.11.0-M8/bundles/scala-xml_2.11.0-M8-1.0.0-RC7.jar]
@dacr
Copy link
Contributor Author

dacr commented Feb 28, 2014

Looks like the dependency comes from scala-compiler/ivy-2.11.0-M8.xml :

       <dependencies>
                <dependency org="org.scala-lang" name="scala-library" rev="2.11.0-M8" force="true" conf="compile->compile(*),master(*);runtime->runtime(*)"/>
                <dependency org="org.scala-lang" name="scala-reflect" rev="2.11.0-M8" force="true" conf="compile->compile(*),master(*);runtime->runtime(*)"/>
                <dependency org="org.scala-lang.modules" name="scala-xml_2.11.0-M8" rev="1.0.0-RC7" force="true" conf="compile->compile(*),master(*);runtime->runtime(*)"/>
                <dependency org="org.scala-lang.modules" name="scala-parser-combinators_2.11.0-M8" rev="1.0.0-RC5" force="true" conf="compile->compile(*),master(*);runtime->runtime(*)"/>
                <dependency org="jline" name="jline" rev="2.11" force="true" conf="optional->compile(*),master(*)"/>
        </dependencies>

@jsuereth
Copy link
Member

@adriaanm This is related to the other thread about running scaladoc, it seems but worse. I'll see if I can dive into your classpaths and try to restore some sanity.

@jsuereth
Copy link
Member

Proof of the issue, as well as culprit:

> show test:fullClasspath
[warn] Credentials file /home/jsuereth/.ivy2/.credentials does not exist
[info] List(Attributed(/home/jsuereth/projects/scala/scala-xml/target/scala-2.11.0-M8/test-classes), Attributed(/home/jsuereth/projects/scala/scala-xml/target/scala-2.11.0-M8/classes), Attributed(/home/jsuereth/.ivy2/cache/org.scala-lang/scala-library/jars/scala-library-2.11.0-M8.jar), Attributed(/home/jsuereth/.ivy2/cache/org.scala-lang/scala-compiler/jars/scala-compiler-2.11.0-M8.jar), Attributed(/home/jsuereth/.ivy2/cache/org.scala-lang/scala-reflect/jars/scala-reflect-2.11.0-M8.jar), Attributed(/home/jsuereth/.ivy2/cache/org.scala-lang.modules/scala-xml_2.11.0-M8/bundles/scala-xml_2.11.0-M8-1.0.0-RC7.jar), Attributed(/home/jsuereth/.ivy2/cache/org.scala-lang.modules/scala-parser-combinators_2.11.0-M8/bundles/scala-parser-combinators_2.11.0-M8-1.0.0-RC5.jar), Attributed(/home/jsuereth/.ivy2/cache/junit/junit/jars/junit-4.11.jar), Attributed(/home/jsuereth/.ivy2/cache/org.hamcrest/hamcrest-core/jars/hamcrest-core-1.3.jar), Attributed(/home/jsuereth/.ivy2/cache/com.novocode/junit-interface/jars/junit-interface-0.10.jar), Attributed(/home/jsuereth/.ivy2/cache/junit/junit-dep/jars/junit-dep-4.10.jar), Attributed(/home/jsuereth/.ivy2/cache/org.scala-tools.testing/test-interface/jars/test-interface-0.5.jar))
[success] Total time: 0 s, completed Feb 28, 2014 9:03:29 AM
> libraryDependencies
[info] List(org.scala-lang:scala-library:2.11.0-M8, junit:junit:4.11:test, com.novocode:junit-interface:0.10:test, org.scala-lang:scala-compiler:2.11.0-M8:test)

@jsuereth
Copy link
Member

Ok, this fix is simple. In build.sbt:

// used in CompilerErrors test
libraryDependencies += ("org.scala-lang" % "scala-compiler" % scalaVersion.value % "test").excludeAll(ExclusionRule(organization="org.scala-lang.modules"))

Proof:

show test:fullClasspath
[warn] Credentials file /home/jsuereth/.ivy2/.credentials does not exist
[info] Updating {file:/home/jsuereth/projects/scala/scala-xml/}scala-xml...
[info] Resolving jline#jline;2.11 ...
[info] Done updating.
[info] List(Attributed(/home/jsuereth/projects/scala/scala-xml/target/scala-2.11.0-M8/test-classes), Attributed(/home/jsuereth/projects/scala/scala-xml/target/scala-2.11.0-M8/classes), Attributed(/home/jsuereth/.ivy2/cache/org.scala-lang/scala-library/jars/scala-library-2.11.0-M8.jar), Attributed(/home/jsuereth/.ivy2/cache/org.scala-lang/scala-compiler/jars/scala-compiler-2.11.0-M8.jar), Attributed(/home/jsuereth/.ivy2/cache/org.scala-lang/scala-reflect/jars/scala-reflect-2.11.0-M8.jar), Attributed(/home/jsuereth/.ivy2/cache/junit/junit/jars/junit-4.11.jar), Attributed(/home/jsuereth/.ivy2/cache/org.hamcrest/hamcrest-core/jars/hamcrest-core-1.3.jar), Attributed(/home/jsuereth/.ivy2/cache/com.novocode/junit-interface/jars/junit-interface-0.10.jar), Attributed(/home/jsuereth/.ivy2/cache/junit/junit-dep/jars/junit-dep-4.10.jar), Attributed(/home/jsuereth/.ivy2/cache/org.scala-tools.testing/test-interface/jars/test-interface-0.5.jar))

Locally tests seem to pass. @adriaanm if you want to go this route, I can submit a PR. I think it's what you want, but please validate the resulting classpath.

@dacr
Copy link
Contributor Author

dacr commented Feb 28, 2014

NOK - Just tested on my environment with a modified XMLEventReader source code. I got the same proof result as you for "show test:fullClasspath" sbt command, but the reality is different, when sbt is started with -verbose JVMOptions, I can still see this class loaded from file scala- xml_2.11.0-M8-1.0.0-RC7.jar and not from scala-xml-dacr/target/scala-2.11.0-M8/classes.

[Loaded scala.xml.pull.XMLEventReader from file:/home/work/.ivy2/cache/org.scala-lang.modules/scala-xml_2.11.0-M8/bundles/scala-xml_2.11.0-M8-1.0.0-RC7.jar]

and so my test case is not executed against my modified XMLEventReader class.

@adriaanm
Copy link
Contributor

Ok, thanks for raising this! I consider this a blocker for the Scala 2.11.0 final release.

@adriaanm
Copy link
Contributor

Implied: I'm looking into fixing this and putting in a backstop to make sure we don't break this again. Help much appreciated!

@jsuereth
Copy link
Member

Ok, working build fix: https://github.com/scala/scala-xml/tree/wip/fix-build-classpath (not in my own fork, sorry!)

@dacr
Copy link
Contributor Author

dacr commented Feb 28, 2014

I've cloned your branch :
1/ no changes, tests are OK although I can see that :

$ export SBT_OPTS="-verbose" ; sbt test | grep "XMLEventReader "
[Loaded scala.xml.pull.XMLEventReader from file:/Users/dcr/.ivy2/cache/org.scala-lang.modules/scala-xml_2.11.0-M8/bundles/scala-xml_2.11.0-M8-1.0.0-RC7.jar]

(vi ~/.sbtconfig to remove SBT_OPTS overwrite)

2/ bring back my XMLEventReader and XMLEventReaderTest changes, but the dependency is still here, and the wrong XMLEventReader class is tested.

@adriaanm
Copy link
Contributor

I haven't pushed the second half of my fix (developed with help of @jsuereth):

//// testing:
// used in CompilerErrors test
libraryDependencies += ("org.scala-lang" % "scala-compiler" % scalaVersion.value % "test").exclude("org.scala-lang.modules", s"scala-xml*")

// needed to fix classloader issues (see #20)
// alternatively, manage the scala instance as shown below (commented)
fork in Test := true

@jsuereth
Copy link
Member

Yeah, I still haven't found where scala-xml 2.11.0-M8 is sneaking into the classpath. My guess is it's getting loaded in sbt's Dual-Loader when the scala compiler is run against the code and then when our child classloader (for the project) is used, it gets that parent one.

Two follow ups:

  1. We may want to separate sbt from any attempts to share Scala classloaders between the compiler + the project. What we're doing now breaks scala modularization.
  2. We should resolve the scala-tool configuration for scala modules using "dummy" artifacts so that when there are circular dependencies the current project doesn't get in the way of grabbing necessary jars for bootstrapping.

@dacr
Copy link
Contributor Author

dacr commented Feb 28, 2014

ok sorry I thought it was done. And thanks for the work around, (fork in Test := true).

adriaanm added a commit to adriaanm/scala-xml that referenced this issue Feb 28, 2014
not scala-compiler's scala-xml dependency...

also put int a println to show the version we're testing to prevent regression
adriaanm added a commit that referenced this issue Feb 28, 2014
Fix #20: test locally built scala.xml classes
gourlaysama added a commit to gourlaysama/scala-parser-combinators that referenced this issue Mar 8, 2014
Somehow, the scala-parser-combinators classes used to run tests were
not the local ones, even though there is no dependency on scala-compiler
nor on anything that depends on scala-parser-combinators.

After some head-scratching, I discovered scala/scala-xml#20. Now I am even
more confused, but at least `fork in Test := true` does prevent this
evil classloader magic.
@gourlaysama
Copy link
Member

It seems like this is also happening with scala-parser-combinators. After desperately trying to make a new test pass, I realized the wrong classfiles were tested... see scala/scala-parser-combinators#13.

adriaanm added a commit to adriaanm/sbt-scala-modules that referenced this issue Mar 8, 2014
Forking is needed to fix scalac classloader issues (see scala/scala-xml#20)
that conflate the compiler's run-time classpath and the compilation classpath.

The run-time (boot)classpath leaks into the compilation classpath,
so that scalac see classes that are used to run it
as classes used to compile against...

Forking uses a minimal classpath, so this craziness is avoided.

Alternatively, we could manage the scala instance as shown at the end of this file (commented)
@drewboardman
Copy link

I'm still running into this bug.

@SethTisue
Copy link
Member

SethTisue commented Nov 19, 2019

@drewboardman can you be more specific?

there are a lot of tickets in this area; I made a (probably not complete) list at #195 (comment)

you are using sbt 1.1.2 or newer, I hope?

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

No branches or pull requests

6 participants